Re: [PATCH net-next 1/2] net: dsa: tag_brcm: add support for legacy tags

[Jonas Gorski]

On Wed, 17 Mar 2021 at 10:16, Álvaro Fernández Rojas  wrote:
>
> Hi Vladimir,
>
> > El 15 mar 2021, a las 22:28, Vladimir Oltean  escribió:
> >
> > On Mon, Mar 15, 2021 at 03:27:35PM +0100, Álvaro Fernández Rojas wrote:
> >> Add support for legacy Broadcom tags, which are similar to 
> >> DSA_TAG_PROTO_BRCM.
> >> These tags are used on BCM5325, BCM5365 and BCM63xx switches.
> >>
> >> Signed-off-by: Álvaro Fernández Rojas 
> >> ---
> >> include/net/dsa.h  |  2 +
> >> net/dsa/Kconfig|  7 
> >> net/dsa/tag_brcm.c | 96 ++
> >> 3 files changed, 105 insertions(+)
> >>
> >> diff --git a/include/net/dsa.h b/include/net/dsa.h
> >> index 83a933e563fe..dac303edd33d 100644
> >> --- a/include/net/dsa.h
> >> +++ b/include/net/dsa.h
> >> @@ -49,10 +49,12 @@ struct phylink_link_state;
> >> #define DSA_TAG_PROTO_XRS700X_VALUE  19
> >> #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE 20
> >> #define DSA_TAG_PROTO_SEVILLE_VALUE  21
> >> +#define DSA_TAG_PROTO_BRCM_LEGACY_VALUE 22
> >>
> >> enum dsa_tag_protocol {
> >>  DSA_TAG_PROTO_NONE  = DSA_TAG_PROTO_NONE_VALUE,
> >>  DSA_TAG_PROTO_BRCM  = DSA_TAG_PROTO_BRCM_VALUE,
> >> +DSA_TAG_PROTO_BRCM_LEGACY   = DSA_TAG_PROTO_BRCM_LEGACY_VALUE,
> >
> > Is there no better qualifier for this tagging protocol name than "legacy"?
>
> It’s always referred to as “legacy”, so that’s what I used.
> Maybe @Florian can suggest a better name for this...

Broadcom refers to both as "the BRCM tag" or "the Broadcom Management
Header/Tag" in documentation with no versioning at all.

Codewise, the brcm963xx code names the old one BRCM_TAG and the newer
one BRCM_TAG_TYPE2. Not really better IMHO.

Maybe BRCM_OLD? less characters than Legacy, and doesn't need to be abbreviated.

To make matters worse, there seem to exist different versions of the
tag variants where some opcodes mean different things, e.g. BCM5325
might set the opcode to 1 for Multicast frames.

I would probably suggest enabling it only for switch models we
verified to be working with it.

On a different side node, should the dsa_tag_protocol be ordered
numerically, i.e. should DSA_TAG_PROTO_BRCM_PREPEND be the last one
since it is the highest with 22?

> >
> >>  DSA_TAG_PROTO_BRCM_PREPEND  = DSA_TAG_PROTO_BRCM_PREPEND_VALUE,
> >>  DSA_TAG_PROTO_DSA   = DSA_TAG_PROTO_DSA_VALUE,
> >>  DSA_TAG_PROTO_EDSA  = DSA_TAG_PROTO_EDSA_VALUE,
> >> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> >> index 58b8fc82cd3c..aaf8a452fd5b 100644
> >> --- a/net/dsa/Kconfig
> >> +++ b/net/dsa/Kconfig
> >> @@ -48,6 +48,13 @@ config NET_DSA_TAG_BRCM
> >>Say Y if you want to enable support for tagging frames for the
> >>Broadcom switches which place the tag after the MAC source address.
> >>
> >> +config NET_DSA_TAG_BRCM_LEGACY
> >> +tristate "Tag driver for Broadcom legacy switches using in-frame 
> >> headers"
> >
> > Aren't all headers in-frame?
>
> I copied that from NET_DSA_TAG_BRCM:
> https://github.com/torvalds/linux/blob/1df27313f50a57497c1faeb6a6ae4ca939c85a7d/net/dsa/Kconfig#L45
>
> Do you want me to change it to "Tag driver for Broadcom legacy switches” or  
> “Legacy tag driver for Broadcom switches"?

This means that the tag is inserted after the SRC/DST mac addresses,
in contrast to BRCM_PREPEND that gets prepended to the full frame.

> >> +select NET_DSA_TAG_BRCM_COMMON
> >> +help
> >> +  Say Y if you want to enable support for tagging frames for the
> >> +  Broadcom legacy switches which place the tag after the MAC source
> >> +  address.
> >>
> >> config NET_DSA_TAG_BRCM_PREPEND
> >>  tristate "Tag driver for Broadcom switches using prepended headers"
> >> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
> >> index e2577a7dcbca..9dbff771c9b3 100644
> >> --- a/net/dsa/tag_brcm.c
> >> +++ b/net/dsa/tag_brcm.c
> >> @@ -9,9 +9,23 @@
> >> #include 
> >> #include 
> >> #include 
> >> +#include 
> >>
> >> #include "dsa_priv.h"
> >>
> >> +struct bcm_legacy_tag {
> >> +uint16_t type;
> >> +#define BRCM_LEG_TYPE   0x8874
> >> +
> >> +uint32_t tag;
> >> +#define BRCM_LEG_TAG_PORT_ID(0xf)
> >> +#define BRCM_LEG_TAG_MULTICAST  (1 << 29)
> >> +#define BRCM_LEG_TAG_EGRESS (2 << 29)
> >> +#define BRCM_LEG_TAG_INGRESS(3 << 29)
> >> +} __attribute__((packed));
> >> +
> >> +#define BRCM_LEG_TAG_LENsizeof(struct bcm_legacy_tag)
> >> +
> >
> > As Florian pointed out, tagging protocol parsing should be
> > endian-independent, and mapping a struct over the frame header is pretty
> > much not that.
>
> Ok, I will change that in v2.
>
> >
> >> /* This tag length is 4 bytes, older ones were 6 bytes, we do not
> >>  * handle them

You might want to update this comment, since you now handle them ;-)

Best Regards
Jonas

Re: [PATCH v4 9/9] Input: add IOC3 serio driver

[Jonas Gorski]

On Wed, 14 Aug 2019 at 16:37, Thomas Bogendoerfer  wrote:
>
> On Wed, 14 Aug 2019 15:20:14 +0200
> Jonas Gorski  wrote:
>
> > > +   d = devm_kzalloc(>dev, sizeof(*d), GFP_KERNEL);
> >
> > >dev => dev
>
> will change.
>
> >
> > > +   if (!d)
> > > +   return -ENOMEM;
> > > +
> > > +   sk = kzalloc(sizeof(*sk), GFP_KERNEL);
> >
> > any reason not to devm_kzalloc this as well? Then you won't need to
> > manually free it in the error cases.
>
> it has different life time than the device, so it may not allocated
> via devm_kzalloc
>
> > > +static int ioc3kbd_remove(struct platform_device *pdev)
> > > +{
> > > +   struct ioc3kbd_data *d = platform_get_drvdata(pdev);
> > > +
> > > +   devm_free_irq(>dev, d->irq, d);
> > > +   serio_unregister_port(d->kbd);
> > > +   serio_unregister_port(d->aux);
> > > +   return 0;
> > > +}
> >
> > and on that topic, won't you need to kfree d->kbd and d->aux here?
>
> that's done in serio_release_port() by the serio core.

i see. But in that case, don't the kfree's after the
serio_unregister_port's in the error path of the .probe function cause
a double free?


Regards
Jonas

Re: [PATCH v4 9/9] Input: add IOC3 serio driver

[Jonas Gorski]

Hi,

On Fri, 9 Aug 2019 at 12:33, Thomas Bogendoerfer  wrote:
>
> This patch adds a platform driver for supporting keyboard and mouse
> interface of SGI IOC3 chips.
>
> Signed-off-by: Thomas Bogendoerfer 
> ---
>  drivers/input/serio/Kconfig   |  10 +++
>  drivers/input/serio/Makefile  |   1 +
>  drivers/input/serio/ioc3kbd.c | 163 
> ++
>  3 files changed, 174 insertions(+)
>  create mode 100644 drivers/input/serio/ioc3kbd.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index f3e18f8ef9ca..373a1646019e 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -165,6 +165,16 @@ config SERIO_MACEPS2
>   To compile this driver as a module, choose M here: the
>   module will be called maceps2.
>
> +config SERIO_SGI_IOC3
> +   tristate "SGI IOC3 PS/2 controller"
> +   depends on SGI_MFD_IOC3
> +   help
> + Say Y here if you have an SGI Onyx2, SGI Octane or IOC3 PCI card
> + and you want to attach and use a keyboard, mouse, or both.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ioc3kbd.
> +
>  config SERIO_LIBPS2
> tristate "PS/2 driver library"
> depends on SERIO_I8042 || SERIO_I8042=n
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 67950a5ccb3f..6d97bad7b844 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_HIL_MLC) += hp_sdc_mlc.o hil_mlc.o
>  obj-$(CONFIG_SERIO_PCIPS2) += pcips2.o
>  obj-$(CONFIG_SERIO_PS2MULT)+= ps2mult.o
>  obj-$(CONFIG_SERIO_MACEPS2)+= maceps2.o
> +obj-$(CONFIG_SERIO_SGI_IOC3)   += ioc3kbd.o
>  obj-$(CONFIG_SERIO_LIBPS2) += libps2.o
>  obj-$(CONFIG_SERIO_RAW)+= serio_raw.o
>  obj-$(CONFIG_SERIO_AMS_DELTA)  += ams_delta_serio.o
> diff --git a/drivers/input/serio/ioc3kbd.c b/drivers/input/serio/ioc3kbd.c
> new file mode 100644
> index ..6840e3c23fed
> --- /dev/null
> +++ b/drivers/input/serio/ioc3kbd.c
> @@ -0,0 +1,163 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 PS/2 controller driver for linux
> + *
> + * Copyright (C) 2019 Thomas Bogendoerfer 
> + *
> + * Based on code Copyright (C) 2005 Stanislaw Skowronek 
> 
> + *   Copyright (C) 2009 Johannes Dickgreber 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +struct ioc3kbd_data {
> +   struct ioc3_serioregs __iomem *regs;
> +   struct serio *kbd, *aux;
> +   int irq;
> +};
> +
> +static int ioc3kbd_write(struct serio *dev, u8 val)
> +{
> +   struct ioc3kbd_data *d = dev->port_data;
> +   unsigned long timeout = 0;
> +   u32 mask;
> +
> +   mask = (dev == d->aux) ? KM_CSR_M_WRT_PEND : KM_CSR_K_WRT_PEND;
> +   while ((readl(>regs->km_csr) & mask) && (timeout < 1000)) {
> +   udelay(100);
> +   timeout++;
> +   }
> +
> +   if (timeout >= 1000)
> +   return -ETIMEDOUT;
> +
> +   writel(val, dev == d->aux ? >regs->m_wd : >regs->k_wd);
> +
> +   return 0;
> +}
> +
> +static irqreturn_t ioc3kbd_intr(int itq, void *dev_id)
> +{
> +   struct ioc3kbd_data *d = dev_id;
> +   u32 data_k, data_m;
> +
> +   data_k = readl(>regs->k_rd);
> +   data_m = readl(>regs->m_rd);
> +
> +   if (data_k & KM_RD_VALID_0)
> +   serio_interrupt(d->kbd, (data_k >> KM_RD_DATA_0_SHIFT) & 0xff,
> +   0);
> +   if (data_k & KM_RD_VALID_1)
> +   serio_interrupt(d->kbd, (data_k >> KM_RD_DATA_1_SHIFT) & 0xff,
> +   0);
> +   if (data_k & KM_RD_VALID_2)
> +   serio_interrupt(d->kbd, (data_k >> KM_RD_DATA_2_SHIFT) & 0xff,
> +   0);
> +   if (data_m & KM_RD_VALID_0)
> +   serio_interrupt(d->aux, (data_m >> KM_RD_DATA_0_SHIFT) & 0xff,
> +   0);
> +   if (data_m & KM_RD_VALID_1)
> +   serio_interrupt(d->aux, (data_m >> KM_RD_DATA_1_SHIFT) & 0xff,
> +   0);
> +   if (data_m & KM_RD_VALID_2)
> +   serio_interrupt(d->aux, (data_m >> KM_RD_DATA_2_SHIFT) & 0xff,
> +   0);
> +
> +   return 0;
> +}
> +
> +static int ioc3kbd_probe(struct platform_device *pdev)
> +{
> +   struct ioc3_serioregs __iomem *regs;
> +   struct device *dev = >dev;
> +   struct ioc3kbd_data *d;
> +   struct serio *sk, *sa;
> +   int irq, ret;
> +
> +   regs = devm_platform_ioremap_resource(pdev, 0);
> +   if (IS_ERR(regs))
> +   return PTR_ERR(regs);
> +
> +   irq = platform_get_irq(pdev, 0);
> +   if (irq < 0)
> +   return -ENXIO;
> +
> +   d = devm_kzalloc(>dev, sizeof(*d), GFP_KERNEL);

>dev => dev

> +   if (!d)
> +   

Re: [PATCH] mailmap: reroute openwrt.org email addresses

[Jonas Gorski]

On Sat, 4 May 2019 at 12:18, Jonas Gorski  wrote:
>
> Since the fork and remerge of LEDE and OpenWrt.org, use of @openwrt.org
> addresses has been discouraged, and some addresses aren't even working
> anymore. To avoid patches being overlooked add appropriate mappings
> based on recent commit history in Linux and OpenWrt.

Great, one instantly replied with no such address, I'll try to get a
better one and send a V2.

Please ignore/drop.

Regards
Jonas

[PATCH] mailmap: reroute openwrt.org email addresses

[Jonas Gorski]

Since the fork and remerge of LEDE and OpenWrt.org, use of @openwrt.org
addresses has been discouraged, and some addresses aren't even working
anymore. To avoid patches being overlooked add appropriate mappings
based on recent commit history in Linux and OpenWrt.

While at it, also update the addresses in MAINTAINERS.

Signed-off-by: Jonas Gorski 
---
 .mailmap| 9 +
 MAINTAINERS | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/.mailmap b/.mailmap
index ae2bcad06f4b..842f69b019d4 100644
--- a/.mailmap
+++ b/.mailmap
@@ -58,14 +58,17 @@ Douglas Gilbert 
 Ed L. Cashin 
 Evgeniy Polyakov 
 Felipe W Damasio 
+Felix Fietkau  
 Felix Kuhling 
 Felix Moeller 
 Filipe Lautert 
+Florian Fainelli  
 Franck Bui-Huu 
 Frank Rowand  
 Frank Rowand  
 Frank Rowand  
 Frank Zago 
+Gabor Juhos  
 Greg Kroah-Hartman 
 Greg Kroah-Hartman 
 Greg Kroah-Hartman 
@@ -94,13 +97,16 @@ Jens Axboe 
 Jens Osterkamp 
 Johan Hovold  
 Johan Hovold  
+John Crispin  
 John Paul Adrian Glaubitz 
 John Stultz 
+Jonas Gorski  
  
  
  
  
  
+Jo-Philipp Wich  
 Juha Yrjola 
 Juha Yrjola 
 Juha Yrjola 
@@ -117,6 +123,7 @@ Leonid I Ananiev 
 Linas Vepstas 
 Linus Lüssing  
 Linus Lüssing  
+Luka Perkov  
 Maciej W. Rozycki  
 Marcin Nowakowski  
 Mark Brown 
@@ -124,6 +131,7 @@ Mark Yao  
 Martin Kepplinger  
 Martin Kepplinger  
 Mathieu Othacehe 
+Matteo Croce  
 Matthew Wilcox  
 Matthew Wilcox  
 Matthew Wilcox  
@@ -199,6 +207,7 @@ Shuah Khan  
 Simon Kelley 
 Stéphane Witzmann 
 Stephen Hemminger 
+Steven Barth  
 Subash Abhinov Kasiviswanathan 
 Subhash Jadavani 
 Sudeep Holla  Sudeep KarkadaNagesha 

diff --git a/MAINTAINERS b/MAINTAINERS
index 057a72a85156..6ab77d69842d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9736,7 +9736,7 @@ F:drivers/leds/leds-mt6323.c
 F: Documentation/devicetree/bindings/leds/leds-mt6323.txt
 
 MEDIATEK ETHERNET DRIVER
-M: Felix Fietkau 
+M: Felix Fietkau 
 M: John Crispin 
 M: Sean Wang 
 M: Nelson Chang 
@@ -13048,7 +13048,7 @@ T:  git 
git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
 F: tools/testing/selftests/rcutorture
 
 RDC R-321X SoC
-M: Florian Fainelli 
+M: Florian Fainelli 
 S: Maintained
 
 RDC R6040 FAST ETHERNET DRIVER
-- 
2.13.2

Re: [PATCH] modpost: make KBUILD_MODPOST_WARN also configurable for external modules

[Jonas Gorski]

Hi,

本当に申し訳ありません, I got sidetracked and completely forgot about it. I
actually still have my old tree with the suggested changes for v2.

On Tue, 9 Apr 2019 at 11:01, Masahiro Yamada
 wrote:
>
> Hi.
>
> On Mon, Apr 8, 2019 at 5:03 PM Wiebe, Wladislav (Nokia - DE/Ulm)
>  wrote:
> >
> > Hi!
> >
> > On 07.04.2019 11:04, Masahiro Yamada wrote:
> > > (+CC Jonas Gorski)
> > >
> > >
> > > On Tue, Mar 26, 2019 at 6:58 PM Wiebe, Wladislav (Nokia - DE/Ulm)
> > >  wrote:
> > >>
> > >> Commit ea837f1c0503 ("kbuild: make modpost processing configurable")
> > >> was intended to give KBUILD_MODPOST_WARN flexibility to be configurable.
> > >> Right now KBUILD_MODPOST_WARN gets just ignored when KBUILD_EXTMOD is
> > >> set which happens per default when building modules out of the tree.
> > >>
> > >> This change gives the opportunity to define module build behaving also
> > >> in case of out of tree builds and default will become exit on error.
> > >> Errors which can be detected by the build should be trapped out of the 
> > >> box
> > >> there, unless somebody wants to notice broken stuff later at runtime.
> > >
> > > If an external module refers to a symbol
> > > provided by another external module,
> > > this patch will turn the warning into the error by default,
> > > which is probably a bad idea.
> >
> > Indeed, exactly this should happen. You should fix your external module
> > dependencies by providing their symbols. Please use e.g.
> > KBUILD_EXTRA_SYMBOLS instead of converting errors to warning and hoping
> > that things will work.
>
> I know this option.
> What I want to say is, this patch changes the behavior, and
> may annoy some people.
>
> > Or how do you want to make sure module A still
> > delivers all symbols needed by module B after an update/changes?
> > Manually comparing the logs after an update or waiting until it turns
> > out broken during run-time? I wouldn't say this is a good idea :-)
>
> OK, so the commit log should record both the behavior change
> and workarounds.
>
> - If an external module being built refers to symbols
>   in another external module, Kbuild previously showed a warning,
>   but going forward it will turn it into an error.
>
> - To work around this, you should pass a symbol table via 
> KBUILD_EXTRA_SYMBOLS,
>   or KBUILD_EXTRA_SYMBOLS=1 to turn the error into a warning.

This sounds like a good middle ground. When you do the effort of
setting KBUILD_EXTRA_SYMBOLS, you are likely interested in proper
dependency resolution and being notified if it fails.

I would probably still use KBUILD_MODPOST_WARN=0 to force the warnings
as errors, to have it as the central switch for the behaviour. So the
behaviour would then become

- If KBUILD_MODPOST_WARN is set to any value, set -w accordingly
- else, set -w only when building for an external module and
KBUILD_EXTRA_SYMBOLS is empty

or

ifdef KBUILD_MODPOST_WARN
KBUILD_MODPOST_WARN:=$(filter-out 0,$(KBUILD_MODPOST_WARN))
else
KBUILD_MODPOST_WARN:=$(if $(KBUILD_EXTMOD),$(if $(KBUILD_EXTRA_SYMBOLS),,1))
endif

(completely untested)


Regards
Jonas

Re: [PATCH] MIPS: ingenic: Add support for appended devicetree

[Jonas Gorski]

On Thu, 21 Feb 2019 at 21:39, Paul Cercueil  wrote:
>
> Add support for booting the kernel from an externally-appended
> devicetree, if no devicetree was built-in.
>
> Signed-off-by: Paul Cercueil 
> ---
>  arch/mips/Kconfig|  2 +-
>  arch/mips/jz4740/setup.c | 14 +++---
>  2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index a84c24d894aa..8b7ea9062198 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -391,7 +391,7 @@ config MACH_INGENIC
> select GPIOLIB
> select COMMON_CLK
> select GENERIC_IRQ_CHIP
> -   select BUILTIN_DTB
> +   select BUILTIN_DTB if MIPS_NO_APPENDED_DTB
> select USE_OF
> select LIBFDT
>
> diff --git a/arch/mips/jz4740/setup.c b/arch/mips/jz4740/setup.c
> index afb40f8bce96..5c00064937c4 100644
> --- a/arch/mips/jz4740/setup.c
> +++ b/arch/mips/jz4740/setup.c
> @@ -31,6 +31,7 @@
>
>  #define JZ4740_EMC_SDRAM_CTRL 0x80
>
> +extern const char __appended_dtb;

Does this build/link with MIPS_NO_APPENDED_DTB? I would assume it
won't be able to resolve the symbol in that case.

You can also just use fw_passed_dtb from asm/bootinfo.h, which will be
populated automatically from fw_args (if UHI) or __appended_dtb (if
present and valid)[1], without having to care where it came from.

Regards
Jonas

[1] https://elixir.bootlin.com/linux/latest/source/arch/mips/kernel/head.S#L96

[PATCH] MIPS: BCM63XX: provide DMA masks for ethernet devices

[Jonas Gorski]

The switch to the generic dma ops made dma masks mandatory, breaking
devices having them not set. In case of bcm63xx, it broke ethernet with
the following warning when trying to up the device:

[2.633123] [ cut here ]
[2.637949] WARNING: CPU: 0 PID: 325 at ./include/linux/dma-mapping.h:516 
bcm_enetsw_open+0x160/0xbbc
[2.647423] Modules linked in: gpio_button_hotplug
[2.652361] CPU: 0 PID: 325 Comm: ip Not tainted 4.19.16 #0
[2.658080] Stack : 8052 804cd3ec   804ccc00 87085bdc 
87d3f9d4 804f9a17
[2.666707] 8049cf18 0145 80a942a0 0204 80ac 10008400 
87085b90 eb3d5ab7
[2.675325]   80ac 22b0   
0007 
[2.683954] 007a 8050 0013b381  8000  
804a1664 80289878
[2.692572] 0009 0204 80ac 0200 0002  
 80a9
[2.701191] ...
[2.703701] Call Trace:
[2.706244] [<8001f3c8>] show_stack+0x58/0x100
[2.710840] [<800336e4>] __warn+0xe4/0x118
[2.715049] [<800337d4>] warn_slowpath_null+0x48/0x64
[2.720237] [<80289878>] bcm_enetsw_open+0x160/0xbbc
[2.725347] [<802d1d4c>] __dev_open+0xf8/0x16c
[2.729913] [<802d20cc>] __dev_change_flags+0x100/0x1c4
[2.735290] [<802d21b8>] dev_change_flags+0x28/0x70
[2.740326] [<803539e0>] devinet_ioctl+0x310/0x7b0
[2.745250] [<80355fd8>] inet_ioctl+0x1f8/0x224
[2.749939] [<802af290>] sock_ioctl+0x30c/0x488
[2.754632] [<80112b34>] do_vfs_ioctl+0x740/0x7dc
[2.759459] [<80112c20>] ksys_ioctl+0x50/0x94
[2.763955] [<800240b8>] syscall_common+0x34/0x58
[2.768782] ---[ end trace fb1a6b14d74e28b6 ]---
[2.773544] bcm63xx_enetsw bcm63xx_enetsw.0: cannot allocate rx ring 512

Fix this by adding appropriate DMA masks for the platform devices.

Fixes: f8c55dc6e828 ("MIPS: use generic dma noncoherent ops for simple 
noncoherent platforms")
Signed-off-by: Jonas Gorski 
---
 arch/mips/bcm63xx/dev-enet.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/mips/bcm63xx/dev-enet.c b/arch/mips/bcm63xx/dev-enet.c
index 07b4c65a88a4..8e73d65f3480 100644
--- a/arch/mips/bcm63xx/dev-enet.c
+++ b/arch/mips/bcm63xx/dev-enet.c
@@ -70,6 +70,8 @@ static struct platform_device bcm63xx_enet_shared_device = {
 
 static int shared_device_registered;
 
+static u64 enet_dmamask = DMA_BIT_MASK(32);
+
 static struct resource enet0_res[] = {
{
.start  = -1, /* filled at runtime */
@@ -99,6 +101,8 @@ static struct platform_device bcm63xx_enet0_device = {
.resource   = enet0_res,
.dev= {
.platform_data = _pd,
+   .dma_mask = _dmamask,
+   .coherent_dma_mask = DMA_BIT_MASK(32),
},
 };
 
@@ -131,6 +135,8 @@ static struct platform_device bcm63xx_enet1_device = {
.resource   = enet1_res,
.dev= {
.platform_data = _pd,
+   .dma_mask = _dmamask,
+   .coherent_dma_mask = DMA_BIT_MASK(32),
},
 };
 
@@ -157,6 +163,8 @@ static struct platform_device bcm63xx_enetsw_device = {
.resource   = enetsw_res,
.dev= {
.platform_data = _pd,
+   .dma_mask = _dmamask,
+   .coherent_dma_mask = DMA_BIT_MASK(32),
},
 };
 
-- 
2.13.2

Re: [PATCH 05/10] staging: mt7621-mmc: Use pinctrl subsystem to select SDXC pin mode

[Jonas Gorski]

On Wed, 20 Feb 2019 at 06:18, George Hilliard
 wrote:
>
> The driver previously grabbed the SD pins for itself, ignoring the pin
> controller.  Replace this direct register access with appropriate calls
> to the pinctrl subsystem.
>
> This also allows this driver to work on related devices that have a
> different pin controller mapping, such as the MT7688.  The hardcoded
> bit index was incorrect on that device.
>
> This change could break SD controller functionality on existing devices
> whose device trees do not specify a pin controller and state for the SD
> node.
>
> Signed-off-by: George Hilliard 
> ---
>  drivers/staging/mt7621-mmc/sd.c | 28 ++--
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/mt7621-mmc/sd.c b/drivers/staging/mt7621-mmc/sd.c
> index a7c7ec0d7bbb..97ed7510e96d 100644
> --- a/drivers/staging/mt7621-mmc/sd.c
> +++ b/drivers/staging/mt7621-mmc/sd.c
> @@ -38,6 +38,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>
>  #include 
> @@ -1599,6 +1600,8 @@ static int msdc_drv_probe(struct platform_device *pdev)
> struct msdc_host *host;
> struct msdc_hw *hw;
> int ret;
> +   struct pinctrl *pctrl;
> +   struct pinctrl_state *pins_default;
>
> hw = _hw;
>
> @@ -1671,6 +1674,25 @@ static int msdc_drv_probe(struct platform_device *pdev)
>
> host->mrq = NULL;
>
> +   /* Read pin control settings from device tree */
> +   pctrl = devm_pinctrl_get(>dev);
> +   if (IS_ERR(pctrl)) {
> +   ret = PTR_ERR(pctrl);
> +   dev_err(>dev, "Cannot find pinctrl in device tree\n");
> +   goto host_free;
> +   }
> +
> +   pins_default = pinctrl_lookup_state(pctrl, PINCTRL_STATE_DEFAULT);
> +   if (IS_ERR(pins_default)) {
> +   ret = PTR_ERR(pins_default);
> +   dev_err(>dev, "Cannot find pinctrl state default\n");
> +   goto host_free;
> +   }
> +
> +   ret = pinctrl_select_state(pctrl, pins_default);
> +   if (ret < 0)
> +   dev_warn(>dev, "Cannot select pinctrl state\n");
> +

Selecting the PINCTRL_STATE_DEFAULT is already automatically done by
the driver core on probe time[1], before calling the probe function,
so this seems redundant here. The only difference is that you enforce
the presence of a pinctrl node, while the core code is fine without
one.


Regards
Jonas

[1] https://elixir.bootlin.com/linux/latest/source/drivers/base/pinctrl.c#L21

[PATCH] hwrng: bcm2835 - fix probe as platform device

[Jonas Gorski]

BCM63XX (MIPS) does not use device tree, so there cannot be any
of_device_id, causing the driver to fail on probe:

[0.904564] bcm2835-rng: probe of bcm63xx-rng failed with error -22

Fix this by checking for match data only if we are probing from device
tree.

Fixes: 8705f24f7b57 ("hwrng: bcm2835 - Enable BCM2835 RNG to work on BCM63xx 
platforms")
Signed-off-by: Jonas Gorski 
---
 drivers/char/hw_random/bcm2835-rng.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/char/hw_random/bcm2835-rng.c 
b/drivers/char/hw_random/bcm2835-rng.c
index 256b0b1d0f26..f759790c3cdb 100644
--- a/drivers/char/hw_random/bcm2835-rng.c
+++ b/drivers/char/hw_random/bcm2835-rng.c
@@ -168,14 +168,16 @@ static int bcm2835_rng_probe(struct platform_device *pdev)
priv->rng.read = bcm2835_rng_read;
priv->rng.cleanup = bcm2835_rng_cleanup;
 
-   rng_id = of_match_node(bcm2835_rng_of_match, np);
-   if (!rng_id)
-   return -EINVAL;
-
-   /* Check for rng init function, execute it */
-   of_data = rng_id->data;
-   if (of_data)
-   priv->mask_interrupts = of_data->mask_interrupts;
+   if (dev_of_node(dev)) {
+   rng_id = of_match_node(bcm2835_rng_of_match, np);
+   if (!rng_id)
+   return -EINVAL;
+
+   /* Check for rng init function, execute it */
+   of_data = rng_id->data;
+   if (of_data)
+   priv->mask_interrupts = of_data->mask_interrupts;
+   }
 
/* register driver */
err = devm_hwrng_register(dev, >rng);
-- 
2.13.2

Re: [PATCH v5 02/12] dt-bindings: pci: add DT docs for Brcmstb PCIe device

[Jonas Gorski]

On 19 September 2018 at 16:31, Jim Quinlan  wrote:
> The DT bindings description of the Brcmstb PCIe device is described.
> This node can be used by almost all Broadcom settop box chips, using
> ARM, ARM64, or MIPS CPU architectures.

Oh, hey, *one* email made it finally through :P

>
> Signed-off-by: Jim Quinlan 
> Acked-by: Rob Herring 
> ---
>  .../devicetree/bindings/pci/brcmstb-pcie.txt   | 59 
> ++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt 
> b/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
> new file mode 100644
> index 000..a1a9ad5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
> @@ -0,0 +1,59 @@
> +Brcmstb PCIe Host Controller Device Tree Bindings
> +
> +Required Properties:
> +- compatible
> +  "brcm,bcm7425-pcie" -- for 7425 family MIPS-based SOCs.
> +  "brcm,bcm7435-pcie" -- for 7435 family MIPS-based SOCs.
> +  "brcm,bcm7445-pcie" -- for 7445 and later ARM based SOCs (not including
> +  the 7278).
> +  "brcm,bcm7278-pcie"  -- for 7278 family ARM-based SOCs.
> +
> +- reg -- the register start address and length for the PCIe reg block.
> +- interrupts -- two interrupts are specified; the first interrupt is for
> + the PCI host controller and the second is for MSI if the built-in
> + MSI controller is to be used.
> +- interrupt-names -- names of the interrupts (above): "pcie" and "msi".
> +- #address-cells -- set to <3>.
> +- #size-cells -- set to <2>.
> +- #interrupt-cells: set to <1>.
> +- interrupt-map-mask and interrupt-map, standard PCI properties to define the
> + mapping of the PCIe interface to interrupt numbers.
> +- ranges: ranges for the PCI memory and I/O regions.
> +- linux,pci-domain -- should be unique per host controller.
> +
> +Optional Properties:
> +- clocks -- phandle of pcie clock.
> +- clock-names -- set to "sw_pcie" if clocks is used.
> +- dma-ranges -- Specifies the inbound memory mapping regions when
> + an "identity map" is not possible.
> +- msi-controller -- this property is typically specified to have the
> + PCIe controller use its internal MSI controller.
> +- msi-parent -- set to use an external MSI interrupt controller.
> +- brcm,enable-ssc -- (boolean) indicates usage of spread-spectrum clocking.
> +- max-link-speed --  (integer) indicates desired generation of link:
> + 1 => 2.5 Gbps (gen1), 2 => 5.0 Gbps (gen2), 3 => 8.0 Gbps (gen3).
> +
> +Example Node:
> +
> +pcie0: pcie@f046 {
> +   reg = <0x0 0xf046 0x0 0x9310>;
> +   interrupts = <0x0 0x0 0x4>;

Your binding says two interrupts, your example has three - what's the
third interrupt for? Also you define the same for MSI and PCIe (I
assume) - is that expected? Are there systems where they are
different? I would expect the msi interrupt to be optional for the
case where its the same as the pcie one, and only required if it is
different.

Also your binding requires an interrupt-names propery, but it's
missing from the example.

> +   compatible = "brcm,bcm7445-pcie";
> +   #address-cells = <3>;
> +   #size-cells = <2>;
> +   ranges = <0x0200 0x 0x 0x 
> 0xc000 0x 0x0800
> + 0x0200 0x 0x0800 0x 
> 0xc800 0x 0x0800>;
> +   #interrupt-cells = <1>;
> +   interrupt-map-mask = <0 0 0 7>;
> +   interrupt-map = <0 0 0 1  0 47 3
> +0 0 0 2  0 48 3
> +0 0 0 3  0 49 3
> +0 0 0 4  0 50 3>;
> +   clocks = <_pcie0>;
> +   clock-names = "sw_pcie";
> +   msi-parent = <>;  /* use PCIe's internal MSI controller 
> */
> +   msi-controller; /* use PCIe's internal MSI controller 
> */
> +   brcm,ssc;
> +   max-link-speed = <1>;
> +   linux,pci-domain = <0>;
> +   };
> --
> 1.9.0.138.g2de3478
>

Regards
Jonas

Re: [PATCH v5 02/12] dt-bindings: pci: add DT docs for Brcmstb PCIe device

[Jonas Gorski]

On 19 September 2018 at 16:31, Jim Quinlan  wrote:
> The DT bindings description of the Brcmstb PCIe device is described.
> This node can be used by almost all Broadcom settop box chips, using
> ARM, ARM64, or MIPS CPU architectures.

Oh, hey, *one* email made it finally through :P

>
> Signed-off-by: Jim Quinlan 
> Acked-by: Rob Herring 
> ---
>  .../devicetree/bindings/pci/brcmstb-pcie.txt   | 59 
> ++
>  1 file changed, 59 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt 
> b/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
> new file mode 100644
> index 000..a1a9ad5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pcie.txt
> @@ -0,0 +1,59 @@
> +Brcmstb PCIe Host Controller Device Tree Bindings
> +
> +Required Properties:
> +- compatible
> +  "brcm,bcm7425-pcie" -- for 7425 family MIPS-based SOCs.
> +  "brcm,bcm7435-pcie" -- for 7435 family MIPS-based SOCs.
> +  "brcm,bcm7445-pcie" -- for 7445 and later ARM based SOCs (not including
> +  the 7278).
> +  "brcm,bcm7278-pcie"  -- for 7278 family ARM-based SOCs.
> +
> +- reg -- the register start address and length for the PCIe reg block.
> +- interrupts -- two interrupts are specified; the first interrupt is for
> + the PCI host controller and the second is for MSI if the built-in
> + MSI controller is to be used.
> +- interrupt-names -- names of the interrupts (above): "pcie" and "msi".
> +- #address-cells -- set to <3>.
> +- #size-cells -- set to <2>.
> +- #interrupt-cells: set to <1>.
> +- interrupt-map-mask and interrupt-map, standard PCI properties to define the
> + mapping of the PCIe interface to interrupt numbers.
> +- ranges: ranges for the PCI memory and I/O regions.
> +- linux,pci-domain -- should be unique per host controller.
> +
> +Optional Properties:
> +- clocks -- phandle of pcie clock.
> +- clock-names -- set to "sw_pcie" if clocks is used.
> +- dma-ranges -- Specifies the inbound memory mapping regions when
> + an "identity map" is not possible.
> +- msi-controller -- this property is typically specified to have the
> + PCIe controller use its internal MSI controller.
> +- msi-parent -- set to use an external MSI interrupt controller.
> +- brcm,enable-ssc -- (boolean) indicates usage of spread-spectrum clocking.
> +- max-link-speed --  (integer) indicates desired generation of link:
> + 1 => 2.5 Gbps (gen1), 2 => 5.0 Gbps (gen2), 3 => 8.0 Gbps (gen3).
> +
> +Example Node:
> +
> +pcie0: pcie@f046 {
> +   reg = <0x0 0xf046 0x0 0x9310>;
> +   interrupts = <0x0 0x0 0x4>;

Your binding says two interrupts, your example has three - what's the
third interrupt for? Also you define the same for MSI and PCIe (I
assume) - is that expected? Are there systems where they are
different? I would expect the msi interrupt to be optional for the
case where its the same as the pcie one, and only required if it is
different.

Also your binding requires an interrupt-names propery, but it's
missing from the example.

> +   compatible = "brcm,bcm7445-pcie";
> +   #address-cells = <3>;
> +   #size-cells = <2>;
> +   ranges = <0x0200 0x 0x 0x 
> 0xc000 0x 0x0800
> + 0x0200 0x 0x0800 0x 
> 0xc800 0x 0x0800>;
> +   #interrupt-cells = <1>;
> +   interrupt-map-mask = <0 0 0 7>;
> +   interrupt-map = <0 0 0 1  0 47 3
> +0 0 0 2  0 48 3
> +0 0 0 3  0 49 3
> +0 0 0 4  0 50 3>;
> +   clocks = <_pcie0>;
> +   clock-names = "sw_pcie";
> +   msi-parent = <>;  /* use PCIe's internal MSI controller 
> */
> +   msi-controller; /* use PCIe's internal MSI controller 
> */
> +   brcm,ssc;
> +   max-link-speed = <1>;
> +   linux,pci-domain = <0>;
> +   };
> --
> 1.9.0.138.g2de3478
>

Regards
Jonas

[PATCH] irqchip/bcm7038-l1: hide cpu offline callback when building for !SMP

[Jonas Gorski]

When compiling bmips with SMP disabled, the build fails with:

drivers/irqchip/irq-bcm7038-l1.o: In function `bcm7038_l1_cpu_offline':
drivers/irqchip/irq-bcm7038-l1.c:242: undefined reference to 
`irq_set_affinity_locked'
make[5]: *** [vmlinux] Error 1

Fix this by adding and setting bcm7038_l1_cpu_offline only when actually
compiling for SMP. It wouldn't have been used anyway, as it requires
CPU_HOTPLUG, which in turn requires SMP.

Fixes: 34c535793bcb ("irqchip/bcm7038-l1: Implement irq_cpu_offline() callback")
Signed-off-by: Jonas Gorski 
---
 drivers/irqchip/irq-bcm7038-l1.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
index faf734ff4cf3..0f6e30e9009d 100644
--- a/drivers/irqchip/irq-bcm7038-l1.c
+++ b/drivers/irqchip/irq-bcm7038-l1.c
@@ -217,6 +217,7 @@ static int bcm7038_l1_set_affinity(struct irq_data *d,
return 0;
 }
 
+#ifdef CONFIG_SMP
 static void bcm7038_l1_cpu_offline(struct irq_data *d)
 {
struct cpumask *mask = irq_data_get_affinity_mask(d);
@@ -241,6 +242,7 @@ static void bcm7038_l1_cpu_offline(struct irq_data *d)
}
irq_set_affinity_locked(d, _affinity, false);
 }
+#endif
 
 static int __init bcm7038_l1_init_one(struct device_node *dn,
  unsigned int idx,
@@ -293,7 +295,9 @@ static struct irq_chip bcm7038_l1_irq_chip = {
.irq_mask   = bcm7038_l1_mask,
.irq_unmask = bcm7038_l1_unmask,
.irq_set_affinity   = bcm7038_l1_set_affinity,
+#ifdef CONFIG_SMP
.irq_cpu_offline= bcm7038_l1_cpu_offline,
+#endif
 };
 
 static int bcm7038_l1_map(struct irq_domain *d, unsigned int virq,
-- 
2.13.2

[PATCH] irqchip/bcm7038-l1: hide cpu offline callback when building for !SMP

[Jonas Gorski]

When compiling bmips with SMP disabled, the build fails with:

drivers/irqchip/irq-bcm7038-l1.o: In function `bcm7038_l1_cpu_offline':
drivers/irqchip/irq-bcm7038-l1.c:242: undefined reference to 
`irq_set_affinity_locked'
make[5]: *** [vmlinux] Error 1

Fix this by adding and setting bcm7038_l1_cpu_offline only when actually
compiling for SMP. It wouldn't have been used anyway, as it requires
CPU_HOTPLUG, which in turn requires SMP.

Fixes: 34c535793bcb ("irqchip/bcm7038-l1: Implement irq_cpu_offline() callback")
Signed-off-by: Jonas Gorski 
---
 drivers/irqchip/irq-bcm7038-l1.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/irqchip/irq-bcm7038-l1.c b/drivers/irqchip/irq-bcm7038-l1.c
index faf734ff4cf3..0f6e30e9009d 100644
--- a/drivers/irqchip/irq-bcm7038-l1.c
+++ b/drivers/irqchip/irq-bcm7038-l1.c
@@ -217,6 +217,7 @@ static int bcm7038_l1_set_affinity(struct irq_data *d,
return 0;
 }
 
+#ifdef CONFIG_SMP
 static void bcm7038_l1_cpu_offline(struct irq_data *d)
 {
struct cpumask *mask = irq_data_get_affinity_mask(d);
@@ -241,6 +242,7 @@ static void bcm7038_l1_cpu_offline(struct irq_data *d)
}
irq_set_affinity_locked(d, _affinity, false);
 }
+#endif
 
 static int __init bcm7038_l1_init_one(struct device_node *dn,
  unsigned int idx,
@@ -293,7 +295,9 @@ static struct irq_chip bcm7038_l1_irq_chip = {
.irq_mask   = bcm7038_l1_mask,
.irq_unmask = bcm7038_l1_unmask,
.irq_set_affinity   = bcm7038_l1_set_affinity,
+#ifdef CONFIG_SMP
.irq_cpu_offline= bcm7038_l1_cpu_offline,
+#endif
 };
 
 static int bcm7038_l1_map(struct irq_domain *d, unsigned int virq,
-- 
2.13.2

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 6 March 2018 at 15:20, Mark Brown <broo...@kernel.org> wrote:
> On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
>> On 5 March 2018 at 21:35, Mark Brown <broo...@kernel.org> wrote:
>
>> > It's exposing more capability information but it's in the "how did this
>> > ever work without the fix" range, and I'd worry that this might cause us
>> > to do something like start exercising handling code in client drivers
>> > that had never been tested.  Not that I can immediately see any client
>> > drivers in mainline that actually pay attention...  :/
>
>> I would assume that most spi client drivers use short messages, so
>> they aren't affected unless the max message size is really short.
>> m25p80 on the other hand will do arbitrarily large transfers/reads, so
>> there it was supported first.
>
> There's a bunch of SPI drivers that do firmware downloads which I'd
> expect to be affected, the limit the device has is tiny so it's
> relatively easy to bump into it.  It's very rare for devices to be so
> limited so unfortunately client drivers don't generally check though.

Well, at least for bcm63xx it's very rare to have something other than
a flash chip, a (broadcom) ethernet switch management interface, or a
SLIC/SLAC attached to the SPI controller. And AFAICT of these three
only the flash chip uses large SPI transfers. Furthermore, unless you
have a development board, you won't be able to attach anything
different to it. So the chance to bump into the limits with other
drivers is rather low.

I would assume that this is true for most systems with a limited SPI
controller. I would hope that most board designers are sensible enough
to not add devices that won't work ;-)


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 6 March 2018 at 15:20, Mark Brown  wrote:
> On Tue, Mar 06, 2018 at 02:42:43PM +0100, Jonas Gorski wrote:
>> On 5 March 2018 at 21:35, Mark Brown  wrote:
>
>> > It's exposing more capability information but it's in the "how did this
>> > ever work without the fix" range, and I'd worry that this might cause us
>> > to do something like start exercising handling code in client drivers
>> > that had never been tested.  Not that I can immediately see any client
>> > drivers in mainline that actually pay attention...  :/
>
>> I would assume that most spi client drivers use short messages, so
>> they aren't affected unless the max message size is really short.
>> m25p80 on the other hand will do arbitrarily large transfers/reads, so
>> there it was supported first.
>
> There's a bunch of SPI drivers that do firmware downloads which I'd
> expect to be affected, the limit the device has is tiny so it's
> relatively easy to bump into it.  It's very rare for devices to be so
> limited so unfortunately client drivers don't generally check though.

Well, at least for bcm63xx it's very rare to have something other than
a flash chip, a (broadcom) ethernet switch management interface, or a
SLIC/SLAC attached to the SPI controller. And AFAICT of these three
only the flash chip uses large SPI transfers. Furthermore, unless you
have a development board, you won't be able to attach anything
different to it. So the chance to bump into the limits with other
drivers is rather low.

I would assume that this is true for most systems with a limited SPI
controller. I would hope that most board designers are sensible enough
to not add devices that won't work ;-)


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 5 March 2018 at 21:35, Mark Brown  wrote:
> On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
>> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>
>> >> The bcm63xx SPI controller does not allow manual control of the CS
>> >> lines and will toggle it automatically before and after sending data,
>> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> >> lines aren't available as GPIOs either, we will need to make slave
>> >> drivers aware of this limitation so they can handle them accordingly.
>
>> >This seems really aggressive for stable...
>
>> Why so?
>
> It's exposing more capability information but it's in the "how did this
> ever work without the fix" range, and I'd worry that this might cause us
> to do something like start exercising handling code in client drivers
> that had never been tested.  Not that I can immediately see any client
> drivers in mainline that actually pay attention...  :/

I would assume that most spi client drivers use short messages, so
they aren't affected unless the max message size is really short.
m25p80 on the other hand will do arbitrarily large transfers/reads, so
there it was supported first.

m25p80 supports max_transfer_size since 4,9, and max_message_size
since 4.11 with commit 9e276de6a367cde07c1a63522152985d4e5cca8b. So
that one would need to be backported as well for the max_message_size
being actually meaningful.

tinydrm-helpers also observes max_transfers_size since 4.11 with
commit 9f69eb5c36a644571cca6b2f8dc5f6a7cba04a8b where it was added,
but since this is a larger commit and not just a "bugfix" one, this
doesn't seem like a candidate for backporting.


Regards
Jonas

Re: [PATCH AUTOSEL for 4.9 002/219] spi/bcm63xx: make spi subsystem aware of message size limits

[Jonas Gorski]

On 5 March 2018 at 21:35, Mark Brown  wrote:
> On Mon, Mar 05, 2018 at 08:07:46PM +, Sasha Levin wrote:
>> On Mon, Mar 05, 2018 at 10:23:10AM +, Mark Brown wrote:
>> >On Sat, Mar 03, 2018 at 10:27:56PM +, Sasha Levin wrote:
>
>> >> The bcm63xx SPI controller does not allow manual control of the CS
>> >> lines and will toggle it automatically before and after sending data,
>> >> so we are limited to messages that fit in the FIFO buffer. Since the CS
>> >> lines aren't available as GPIOs either, we will need to make slave
>> >> drivers aware of this limitation so they can handle them accordingly.
>
>> >This seems really aggressive for stable...
>
>> Why so?
>
> It's exposing more capability information but it's in the "how did this
> ever work without the fix" range, and I'd worry that this might cause us
> to do something like start exercising handling code in client drivers
> that had never been tested.  Not that I can immediately see any client
> drivers in mainline that actually pay attention...  :/

I would assume that most spi client drivers use short messages, so
they aren't affected unless the max message size is really short.
m25p80 on the other hand will do arbitrarily large transfers/reads, so
there it was supported first.

m25p80 supports max_transfer_size since 4,9, and max_message_size
since 4.11 with commit 9e276de6a367cde07c1a63522152985d4e5cca8b. So
that one would need to be backported as well for the max_message_size
being actually meaningful.

tinydrm-helpers also observes max_transfers_size since 4.11 with
commit 9f69eb5c36a644571cca6b2f8dc5f6a7cba04a8b where it was added,
but since this is a larger commit and not just a "bugfix" one, this
doesn't seem like a candidate for backporting.


Regards
Jonas

Re: [PATCH v3 5/8] MIPS: mscc: add ocelot dtsi

[Jonas Gorski]

On 16 January 2018 at 11:12, Alexandre Belloni
 wrote:
> Add a device tree include file for the Microsemi Ocelot SoC.
>
> Signed-off-by: Alexandre Belloni 
> ---
>  arch/mips/boot/dts/Makefile |   1 +
>  arch/mips/boot/dts/mscc/Makefile|   4 ++
>  arch/mips/boot/dts/mscc/ocelot.dtsi | 110 
> 
>  3 files changed, 115 insertions(+)
>  create mode 100644 arch/mips/boot/dts/mscc/Makefile
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot.dtsi
>
> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
> index e2c6f131c8eb..1e79cab8e269 100644
> --- a/arch/mips/boot/dts/Makefile
> +++ b/arch/mips/boot/dts/Makefile
> @@ -4,6 +4,7 @@ subdir-y+= cavium-octeon
>  subdir-y   += img
>  subdir-y   += ingenic
>  subdir-y   += lantiq
> +subdir-y   += mscc
>  subdir-y   += mti
>  subdir-y   += netlogic
>  subdir-y   += ni
> diff --git a/arch/mips/boot/dts/mscc/Makefile 
> b/arch/mips/boot/dts/mscc/Makefile
> new file mode 100644
> index ..f0a155a74e02
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/Makefile
> @@ -0,0 +1,4 @@
> +obj-y  += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
> +
> +# Force kbuild to make empty built-in.o if necessary
> +obj-   += dummy.o
> diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi 
> b/arch/mips/boot/dts/mscc/ocelot.dtsi
> new file mode 100644
> index ..b2f936e1fbb9
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/* Copyright (c) 2017 Microsemi Corporation */
> +
> +/ {
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   compatible = "mscc,ocelot";
> +
> +   cpus {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +
> +   mips-hpt-frequency = <25000>;
> +
> +   cpu@0 {
> +   compatible = "mscc,ocelot";

You are using the same compatible string for the whole chip as well as
the cpu core of it, this doesn't seem right.

Also is this really a custom cpu core? Your product brief suggests
this is a "normal" 24KEc MIPS CPU, at least for ocelot-10 (VSC7514).
So something like "mips,mips24KEc" might be more appropriate here.


Regards
Jonas

Re: [PATCH v3 5/8] MIPS: mscc: add ocelot dtsi

[Jonas Gorski]

On 16 January 2018 at 11:12, Alexandre Belloni
 wrote:
> Add a device tree include file for the Microsemi Ocelot SoC.
>
> Signed-off-by: Alexandre Belloni 
> ---
>  arch/mips/boot/dts/Makefile |   1 +
>  arch/mips/boot/dts/mscc/Makefile|   4 ++
>  arch/mips/boot/dts/mscc/ocelot.dtsi | 110 
> 
>  3 files changed, 115 insertions(+)
>  create mode 100644 arch/mips/boot/dts/mscc/Makefile
>  create mode 100644 arch/mips/boot/dts/mscc/ocelot.dtsi
>
> diff --git a/arch/mips/boot/dts/Makefile b/arch/mips/boot/dts/Makefile
> index e2c6f131c8eb..1e79cab8e269 100644
> --- a/arch/mips/boot/dts/Makefile
> +++ b/arch/mips/boot/dts/Makefile
> @@ -4,6 +4,7 @@ subdir-y+= cavium-octeon
>  subdir-y   += img
>  subdir-y   += ingenic
>  subdir-y   += lantiq
> +subdir-y   += mscc
>  subdir-y   += mti
>  subdir-y   += netlogic
>  subdir-y   += ni
> diff --git a/arch/mips/boot/dts/mscc/Makefile 
> b/arch/mips/boot/dts/mscc/Makefile
> new file mode 100644
> index ..f0a155a74e02
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/Makefile
> @@ -0,0 +1,4 @@
> +obj-y  += $(patsubst %.dtb, %.dtb.o, $(dtb-y))
> +
> +# Force kbuild to make empty built-in.o if necessary
> +obj-   += dummy.o
> diff --git a/arch/mips/boot/dts/mscc/ocelot.dtsi 
> b/arch/mips/boot/dts/mscc/ocelot.dtsi
> new file mode 100644
> index ..b2f936e1fbb9
> --- /dev/null
> +++ b/arch/mips/boot/dts/mscc/ocelot.dtsi
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
> +/* Copyright (c) 2017 Microsemi Corporation */
> +
> +/ {
> +   #address-cells = <1>;
> +   #size-cells = <1>;
> +   compatible = "mscc,ocelot";
> +
> +   cpus {
> +   #address-cells = <1>;
> +   #size-cells = <0>;
> +
> +   mips-hpt-frequency = <25000>;
> +
> +   cpu@0 {
> +   compatible = "mscc,ocelot";

You are using the same compatible string for the whole chip as well as
the cpu core of it, this doesn't seem right.

Also is this really a custom cpu core? Your product brief suggests
this is a "normal" 24KEc MIPS CPU, at least for ocelot-10 (VSC7514).
So something like "mips,mips24KEc" might be more appropriate here.


Regards
Jonas

Re: [PATCH] irqchip: Use %px to print pointer value

[Jonas Gorski]

On 9 February 2018 at 17:04, Marc Zyngier  wrote:
> On 09/02/18 15:54, Florian Fainelli wrote:
>> On February 9, 2018 12:51:33 AM PST, Marc Zyngier  
>> wrote:
>>> On 09/02/18 02:10, Jaedon Shin wrote:
 Since commit ad67b74d2469 ("printk: hash addresses printed with %p")
 pointers printed with %p are hashed. Use %px instead of %p to print
 pointer value.

 Signed-off-by: Jaedon Shin 
 ---
  drivers/irqchip/irq-bcm7038-l1.c | 2 +-
  drivers/irqchip/irq-bcm7120-l2.c | 2 +-
  drivers/irqchip/irq-brcmstb-l2.c | 2 +-
  3 files changed, 3 insertions(+), 3 deletions(-)

 diff --git a/drivers/irqchip/irq-bcm7038-l1.c
>>> b/drivers/irqchip/irq-bcm7038-l1.c
 index 55cfb986225b..f604c1d89b3b 100644
 --- a/drivers/irqchip/irq-bcm7038-l1.c
 +++ b/drivers/irqchip/irq-bcm7038-l1.c
 @@ -339,7 +339,7 @@ int __init bcm7038_l1_of_init(struct device_node
>>> *dn,
 goto out_unmap;
 }

 -   pr_info("registered BCM7038 L1 intc (mem: 0x%p, IRQs: %d)\n",
 +   pr_info("registered BCM7038 L1 intc (mem: 0x%px, IRQs: %d)\n",
 intc->cpus[0]->map_base, IRQS_PER_WORD * intc->n_words);

 return 0;
 diff --git a/drivers/irqchip/irq-bcm7120-l2.c
>>> b/drivers/irqchip/irq-bcm7120-l2.c
 index 983640eba418..1cc4dd1d584a 100644
 --- a/drivers/irqchip/irq-bcm7120-l2.c
 +++ b/drivers/irqchip/irq-bcm7120-l2.c
 @@ -318,7 +318,7 @@ static int __init bcm7120_l2_intc_probe(struct
>>> device_node *dn,
 }
 }

 -   pr_info("registered %s intc (mem: 0x%p, parent IRQ(s): %d)\n",
 +   pr_info("registered %s intc (mem: 0x%px, parent IRQ(s): %d)\n",
 intc_name, data->map_base[0], data->num_parent_irqs);

 return 0;
 diff --git a/drivers/irqchip/irq-brcmstb-l2.c
>>> b/drivers/irqchip/irq-brcmstb-l2.c
 index 691d20eb0bec..6760edeeb666 100644
 --- a/drivers/irqchip/irq-brcmstb-l2.c
 +++ b/drivers/irqchip/irq-brcmstb-l2.c
 @@ -262,7 +262,7 @@ static int __init brcmstb_l2_intc_of_init(struct
>>> device_node *np,
 ct->chip.irq_set_wake = irq_gc_set_wake;
 }

 -   pr_info("registered L2 intc (mem: 0x%p, parent irq: %d)\n",
 +   pr_info("registered L2 intc (mem: 0x%px, parent irq: %d)\n",
 base, parent_irq);

 return 0;

>>>
>>> Why is that something useful to do? This just tells you where the
>>> device
>>> is mapped in the VA space, and I doubt that's a useful information,
>>> hashed pointers or not. Am I missing something obvious?
>>
>> No you are right there is not much value in printing the register
>> virtual address (sometimes there is e.g: on MIPS) either we fix the
>> prints to show the physical address of the base register or we could
>> possibly drop the prints entirely.
>
> Displaying the PA can be useful if you have several identical blocks in
> your system and you want to be able to identify them. Given that there
> is probably only one of these controllers per system, the address is
> pretty pointless.

Multiple instances are actually quite common in the STB SoCs, e.g.
bcm7362 has one instance of bcm7038-l1, two instances of bcm7120-l2
and four instances of brcmstb-l2.


Regards
Jonas

P.S: Also What about bcm6345-l1? It also prints it's mapped VAs, not the PAs.

Re: [PATCH] irqchip: Use %px to print pointer value

[Jonas Gorski]

On 9 February 2018 at 17:04, Marc Zyngier  wrote:
> On 09/02/18 15:54, Florian Fainelli wrote:
>> On February 9, 2018 12:51:33 AM PST, Marc Zyngier  
>> wrote:
>>> On 09/02/18 02:10, Jaedon Shin wrote:
 Since commit ad67b74d2469 ("printk: hash addresses printed with %p")
 pointers printed with %p are hashed. Use %px instead of %p to print
 pointer value.

 Signed-off-by: Jaedon Shin 
 ---
  drivers/irqchip/irq-bcm7038-l1.c | 2 +-
  drivers/irqchip/irq-bcm7120-l2.c | 2 +-
  drivers/irqchip/irq-brcmstb-l2.c | 2 +-
  3 files changed, 3 insertions(+), 3 deletions(-)

 diff --git a/drivers/irqchip/irq-bcm7038-l1.c
>>> b/drivers/irqchip/irq-bcm7038-l1.c
 index 55cfb986225b..f604c1d89b3b 100644
 --- a/drivers/irqchip/irq-bcm7038-l1.c
 +++ b/drivers/irqchip/irq-bcm7038-l1.c
 @@ -339,7 +339,7 @@ int __init bcm7038_l1_of_init(struct device_node
>>> *dn,
 goto out_unmap;
 }

 -   pr_info("registered BCM7038 L1 intc (mem: 0x%p, IRQs: %d)\n",
 +   pr_info("registered BCM7038 L1 intc (mem: 0x%px, IRQs: %d)\n",
 intc->cpus[0]->map_base, IRQS_PER_WORD * intc->n_words);

 return 0;
 diff --git a/drivers/irqchip/irq-bcm7120-l2.c
>>> b/drivers/irqchip/irq-bcm7120-l2.c
 index 983640eba418..1cc4dd1d584a 100644
 --- a/drivers/irqchip/irq-bcm7120-l2.c
 +++ b/drivers/irqchip/irq-bcm7120-l2.c
 @@ -318,7 +318,7 @@ static int __init bcm7120_l2_intc_probe(struct
>>> device_node *dn,
 }
 }

 -   pr_info("registered %s intc (mem: 0x%p, parent IRQ(s): %d)\n",
 +   pr_info("registered %s intc (mem: 0x%px, parent IRQ(s): %d)\n",
 intc_name, data->map_base[0], data->num_parent_irqs);

 return 0;
 diff --git a/drivers/irqchip/irq-brcmstb-l2.c
>>> b/drivers/irqchip/irq-brcmstb-l2.c
 index 691d20eb0bec..6760edeeb666 100644
 --- a/drivers/irqchip/irq-brcmstb-l2.c
 +++ b/drivers/irqchip/irq-brcmstb-l2.c
 @@ -262,7 +262,7 @@ static int __init brcmstb_l2_intc_of_init(struct
>>> device_node *np,
 ct->chip.irq_set_wake = irq_gc_set_wake;
 }

 -   pr_info("registered L2 intc (mem: 0x%p, parent irq: %d)\n",
 +   pr_info("registered L2 intc (mem: 0x%px, parent irq: %d)\n",
 base, parent_irq);

 return 0;

>>>
>>> Why is that something useful to do? This just tells you where the
>>> device
>>> is mapped in the VA space, and I doubt that's a useful information,
>>> hashed pointers or not. Am I missing something obvious?
>>
>> No you are right there is not much value in printing the register
>> virtual address (sometimes there is e.g: on MIPS) either we fix the
>> prints to show the physical address of the base register or we could
>> possibly drop the prints entirely.
>
> Displaying the PA can be useful if you have several identical blocks in
> your system and you want to be able to identify them. Given that there
> is probably only one of these controllers per system, the address is
> pretty pointless.

Multiple instances are actually quite common in the STB SoCs, e.g.
bcm7362 has one instance of bcm7038-l1, two instances of bcm7120-l2
and four instances of brcmstb-l2.


Regards
Jonas

P.S: Also What about bcm6345-l1? It also prints it's mapped VAs, not the PAs.

Re: [PATCH 2/8] PCI: host: brcmstb: add DT docs for Brcmstb PCIe device

[Jonas Gorski]

Hi,

On 24 October 2017 at 20:15, Jim Quinlan  wrote:
> The DT bindings description of the Brcmstb PCIe device is described.  This
> node can be used by almost all Broadcom settop box chips, using
> ARM, ARM64, or MIPS CPU architectures.
>
> Signed-off-by: Jim Quinlan 
> ---
>  .../devicetree/bindings/pci/brcmstb-pci.txt| 63 
> ++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt 
> b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt
> new file mode 100644
> index 000..49f9852
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt
> @@ -0,0 +1,63 @@
> +Brcmstb PCIe Host Controller Device Tree Bindings
> +
> +Required Properties:
> +- compatible
> +  "brcm,bcm7425-pcie" -- for 7425 family MIPS-based SOCs.
> +  "brcm,bcm7435-pcie" -- for 7435 family MIPS-based SOCs.
> +  "brcm,bcm7445-pcie" -- for 7445 and later ARM based SOCs (not including
> +  the 7278).
> +  "brcm,bcm7278-pcie"  -- for 7278 family ARM-based SOCs.
> +

(snip)

> +
> +Example Node:
> +
> +pcie0: pcie@f046 {
> +   reg = <0x0 0xf046 0x0 0x9310>;
> +   interrupts = <0x0 0x0 0x4>;
> +   compatible = "brcm,pci-plat-dev";

This is not one of the valid compatibles mentioned above.


Regards
Jonas

Re: [PATCH 2/8] PCI: host: brcmstb: add DT docs for Brcmstb PCIe device

[Jonas Gorski]

Hi,

On 24 October 2017 at 20:15, Jim Quinlan  wrote:
> The DT bindings description of the Brcmstb PCIe device is described.  This
> node can be used by almost all Broadcom settop box chips, using
> ARM, ARM64, or MIPS CPU architectures.
>
> Signed-off-by: Jim Quinlan 
> ---
>  .../devicetree/bindings/pci/brcmstb-pci.txt| 63 
> ++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt
>
> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt 
> b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt
> new file mode 100644
> index 000..49f9852
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt
> @@ -0,0 +1,63 @@
> +Brcmstb PCIe Host Controller Device Tree Bindings
> +
> +Required Properties:
> +- compatible
> +  "brcm,bcm7425-pcie" -- for 7425 family MIPS-based SOCs.
> +  "brcm,bcm7435-pcie" -- for 7435 family MIPS-based SOCs.
> +  "brcm,bcm7445-pcie" -- for 7445 and later ARM based SOCs (not including
> +  the 7278).
> +  "brcm,bcm7278-pcie"  -- for 7278 family ARM-based SOCs.
> +

(snip)

> +
> +Example Node:
> +
> +pcie0: pcie@f046 {
> +   reg = <0x0 0xf046 0x0 0x9310>;
> +   interrupts = <0x0 0x0 0x4>;
> +   compatible = "brcm,pci-plat-dev";

This is not one of the valid compatibles mentioned above.


Regards
Jonas

Re: [PATCH] kbuild: allow making undefined symbols fatal for external modules

[Jonas Gorski]

Hi Masahiro,

On 9 October 2017 at 18:25, Masahiro Yamada
<yamada.masah...@socionext.com> wrote:
> 2017-10-02 19:50 GMT+09:00 Jonas Gorski <jonas.gor...@gmail.com>:
>> By passing appropriate values in KBUILD_EXTRA_SYMBOLS it is possible to
>> make modpost be able to resolve all symbols for external modules, even
>> between to other external modules.
>>
>> Because of this, it might be desirable to make unresolved symbols an
>> error on external module builds as well, to catch missing exports there
>> as well.
>>
>> So add a new flag KBUILD_MODPOST_ERROR which can be set to enable this
>> behaviour. It has no effect on non external module builds.
>>
>> Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
>> ---
>> The main target is to allow distributions (like LEDE) that make use of
>> kernel backports and other out of tree drivers to be able to catch
>> errors like missing EXPORT_SYMBOLs in the kernel early.
>>
>>  scripts/Makefile.modpost | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index 16923ba4b5b1..29f787bf8c3a 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -34,6 +34,8 @@
>>
>>  # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
>>  # symbols in the final module linking stage
>> +# KBUILD_MODPOST_ERROR can be set to error out in case of undefined
>> +# symbols in the final module linking stage for external modules
>>  # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
>>  # This is solely useful to speed up test compiles
>>  PHONY := _modpost
>> @@ -49,6 +51,8 @@ ifneq ($(KBUILD_EXTMOD),)
>>  obj := $(KBUILD_EXTMOD)
>>  src := $(obj)
>>
>> +KBUILD_MODPOST_WARN = $(if $(KBUILD_MODPOST_ERROR),,1)
>> +
>>  # Include the module's Makefile to find KBUILD_EXTRA_SYMBOLS
>>  include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
>>   $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
>> @@ -78,7 +82,7 @@ modpost = scripts/mod/modpost\
>>   $(if $(KBUILD_EXTMOD),-o $(modulesymfile))  \
>>   $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)  \
>>   $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
>> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
>> + $(if $(KBUILD_MODPOST_WARN),-w)
>>
>>  MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>>
>
>
> The motivation looks reasonable to me,
> but having both KBUILD_MODPOST_WARN and KBUILD_MODPOST_ERROR
> is a bit confusing, I think.
>
> (Some people may wonder what would happen if both are set...)
>
>
> So, here is my suggestion.
>
>
> We have only KBUILD_MODPOST_WARN.
>
> The default value is 1 for external module building.
>  0 for kernel building.
>
>
> If you want to change the default behavior,
> you can specify KBUILD_MODPOST_WARN=1 when building kernel,
> or KBUILD_MODPOST_WARN=0 when building external module
> from the command line.

Makes sense, I was thinking too complicated.

>
> Also, you need to update Documentation/kbuild/kbuild.txt

Indeed, I missed that. Will fix in v2.


Regards
Jonas

Re: [PATCH] kbuild: allow making undefined symbols fatal for external modules

[Jonas Gorski]

Hi Masahiro,

On 9 October 2017 at 18:25, Masahiro Yamada
 wrote:
> 2017-10-02 19:50 GMT+09:00 Jonas Gorski :
>> By passing appropriate values in KBUILD_EXTRA_SYMBOLS it is possible to
>> make modpost be able to resolve all symbols for external modules, even
>> between to other external modules.
>>
>> Because of this, it might be desirable to make unresolved symbols an
>> error on external module builds as well, to catch missing exports there
>> as well.
>>
>> So add a new flag KBUILD_MODPOST_ERROR which can be set to enable this
>> behaviour. It has no effect on non external module builds.
>>
>> Signed-off-by: Jonas Gorski 
>> ---
>> The main target is to allow distributions (like LEDE) that make use of
>> kernel backports and other out of tree drivers to be able to catch
>> errors like missing EXPORT_SYMBOLs in the kernel early.
>>
>>  scripts/Makefile.modpost | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
>> index 16923ba4b5b1..29f787bf8c3a 100644
>> --- a/scripts/Makefile.modpost
>> +++ b/scripts/Makefile.modpost
>> @@ -34,6 +34,8 @@
>>
>>  # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
>>  # symbols in the final module linking stage
>> +# KBUILD_MODPOST_ERROR can be set to error out in case of undefined
>> +# symbols in the final module linking stage for external modules
>>  # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
>>  # This is solely useful to speed up test compiles
>>  PHONY := _modpost
>> @@ -49,6 +51,8 @@ ifneq ($(KBUILD_EXTMOD),)
>>  obj := $(KBUILD_EXTMOD)
>>  src := $(obj)
>>
>> +KBUILD_MODPOST_WARN = $(if $(KBUILD_MODPOST_ERROR),,1)
>> +
>>  # Include the module's Makefile to find KBUILD_EXTRA_SYMBOLS
>>  include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
>>   $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
>> @@ -78,7 +82,7 @@ modpost = scripts/mod/modpost\
>>   $(if $(KBUILD_EXTMOD),-o $(modulesymfile))  \
>>   $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)  \
>>   $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
>> - $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
>> + $(if $(KBUILD_MODPOST_WARN),-w)
>>
>>  MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
>>
>
>
> The motivation looks reasonable to me,
> but having both KBUILD_MODPOST_WARN and KBUILD_MODPOST_ERROR
> is a bit confusing, I think.
>
> (Some people may wonder what would happen if both are set...)
>
>
> So, here is my suggestion.
>
>
> We have only KBUILD_MODPOST_WARN.
>
> The default value is 1 for external module building.
>  0 for kernel building.
>
>
> If you want to change the default behavior,
> you can specify KBUILD_MODPOST_WARN=1 when building kernel,
> or KBUILD_MODPOST_WARN=0 when building external module
> from the command line.

Makes sense, I was thinking too complicated.

>
> Also, you need to update Documentation/kbuild/kbuild.txt

Indeed, I missed that. Will fix in v2.


Regards
Jonas

[PATCH] kbuild: allow making undefined symbols fatal for external modules

[Jonas Gorski]

By passing appropriate values in KBUILD_EXTRA_SYMBOLS it is possible to
make modpost be able to resolve all symbols for external modules, even
between to other external modules.

Because of this, it might be desirable to make unresolved symbols an
error on external module builds as well, to catch missing exports there
as well.

So add a new flag KBUILD_MODPOST_ERROR which can be set to enable this
behaviour. It has no effect on non external module builds.

Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
The main target is to allow distributions (like LEDE) that make use of
kernel backports and other out of tree drivers to be able to catch
errors like missing EXPORT_SYMBOLs in the kernel early.

 scripts/Makefile.modpost | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 16923ba4b5b1..29f787bf8c3a 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -34,6 +34,8 @@
 
 # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
 # symbols in the final module linking stage
+# KBUILD_MODPOST_ERROR can be set to error out in case of undefined
+# symbols in the final module linking stage for external modules
 # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
 # This is solely useful to speed up test compiles
 PHONY := _modpost
@@ -49,6 +51,8 @@ ifneq ($(KBUILD_EXTMOD),)
 obj := $(KBUILD_EXTMOD)
 src := $(obj)
 
+KBUILD_MODPOST_WARN = $(if $(KBUILD_MODPOST_ERROR),,1)
+
 # Include the module's Makefile to find KBUILD_EXTRA_SYMBOLS
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
  $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
@@ -78,7 +82,7 @@ modpost = scripts/mod/modpost\
  $(if $(KBUILD_EXTMOD),-o $(modulesymfile))  \
  $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)  \
  $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
- $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_MODPOST_WARN),-w)
 
 MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
-- 
2.13.2

[PATCH] kbuild: allow making undefined symbols fatal for external modules

[Jonas Gorski]

By passing appropriate values in KBUILD_EXTRA_SYMBOLS it is possible to
make modpost be able to resolve all symbols for external modules, even
between to other external modules.

Because of this, it might be desirable to make unresolved symbols an
error on external module builds as well, to catch missing exports there
as well.

So add a new flag KBUILD_MODPOST_ERROR which can be set to enable this
behaviour. It has no effect on non external module builds.

Signed-off-by: Jonas Gorski 
---
The main target is to allow distributions (like LEDE) that make use of
kernel backports and other out of tree drivers to be able to catch
errors like missing EXPORT_SYMBOLs in the kernel early.

 scripts/Makefile.modpost | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index 16923ba4b5b1..29f787bf8c3a 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -34,6 +34,8 @@
 
 # KBUILD_MODPOST_WARN can be set to avoid error out in case of undefined
 # symbols in the final module linking stage
+# KBUILD_MODPOST_ERROR can be set to error out in case of undefined
+# symbols in the final module linking stage for external modules
 # KBUILD_MODPOST_NOFINAL can be set to skip the final link of modules.
 # This is solely useful to speed up test compiles
 PHONY := _modpost
@@ -49,6 +51,8 @@ ifneq ($(KBUILD_EXTMOD),)
 obj := $(KBUILD_EXTMOD)
 src := $(obj)
 
+KBUILD_MODPOST_WARN = $(if $(KBUILD_MODPOST_ERROR),,1)
+
 # Include the module's Makefile to find KBUILD_EXTRA_SYMBOLS
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
  $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
@@ -78,7 +82,7 @@ modpost = scripts/mod/modpost\
  $(if $(KBUILD_EXTMOD),-o $(modulesymfile))  \
  $(if $(CONFIG_DEBUG_SECTION_MISMATCH),,-S)  \
  $(if $(CONFIG_SECTION_MISMATCH_WARN_ONLY),,-E)  \
- $(if $(KBUILD_EXTMOD)$(KBUILD_MODPOST_WARN),-w)
+ $(if $(KBUILD_MODPOST_WARN),-w)
 
 MODPOST_OPT=$(subst -i,-n,$(filter -i,$(MAKEFLAGS)))
 
-- 
2.13.2

Re: [PATCH 4/9] m68k: allow NULL clock for clk_get_rate

[Jonas Gorski]

Hi Greg,

On 18 July 2017 at 14:03, Greg Ungerer <g...@linux-m68k.org> wrote:
> Hi Jonas,
>
> On 18/07/17 20:17, Jonas Gorski wrote:
>>
>> Make the behaviour of clk_get_rate consistent with common clk's
>> clk_get_rate by accepting NULL clocks as parameter. Some device
>> drivers rely on this, and will cause an OOPS otherwise.
>>
>> Fixes: facdf0ed4f59 ("m68knommu: introduce basic clk infrastructure")
>> Cc: Greg Ungerer <g...@linux-m68k.org>
>
>
> Acked-by: Greg Ungerer <g...@linux-m68k.org>
>
> Do you want me to push this via the m68knommu git tree?
> Or are you (or someone) taking the series as a whole?

Please take it through your tree. I totally forgot mentioning in the
cover letter that I'm just a simple patch submitter and don't have my
own tree. Too long ago that I sent a multi-tree patch series last time
... .

Regards
Jonas

Re: [PATCH 4/9] m68k: allow NULL clock for clk_get_rate

[Jonas Gorski]

Hi Greg,

On 18 July 2017 at 14:03, Greg Ungerer  wrote:
> Hi Jonas,
>
> On 18/07/17 20:17, Jonas Gorski wrote:
>>
>> Make the behaviour of clk_get_rate consistent with common clk's
>> clk_get_rate by accepting NULL clocks as parameter. Some device
>> drivers rely on this, and will cause an OOPS otherwise.
>>
>> Fixes: facdf0ed4f59 ("m68knommu: introduce basic clk infrastructure")
>> Cc: Greg Ungerer 
>
>
> Acked-by: Greg Ungerer 
>
> Do you want me to push this via the m68knommu git tree?
> Or are you (or someone) taking the series as a whole?

Please take it through your tree. I totally forgot mentioning in the
cover letter that I'm just a simple patch submitter and don't have my
own tree. Too long ago that I sent a multi-tree patch series last time
... .

Regards
Jonas

[PATCH 2/9] ARM: mmp: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 49cbe78637eb ("[ARM] pxa: add base support for Marvell's PXA168 
processor line")
Cc: Eric Miao <eric.y.m...@gmail.com>
Cc: Haojian Zhuang <haojian.zhu...@gmail.com>
Cc: Russell King <li...@armlinux.org.uk>
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/arm/mach-mmp/clock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mmp/clock.c b/arch/arm/mach-mmp/clock.c
index 28fe64c6e2f5..bdfb113431ec 100644
--- a/arch/arm/mach-mmp/clock.c
+++ b/arch/arm/mach-mmp/clock.c
@@ -83,7 +83,9 @@ unsigned long clk_get_rate(struct clk *clk)
 {
unsigned long rate;
 
-   if (clk->ops->getrate)
+   if (!clk)
+   rate = 0;
+   else if (clk->ops->getrate)
rate = clk->ops->getrate(clk);
else
rate = clk->rate;
-- 
2.11.0

[PATCH 2/9] ARM: mmp: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 49cbe78637eb ("[ARM] pxa: add base support for Marvell's PXA168 
processor line")
Cc: Eric Miao 
Cc: Haojian Zhuang 
Cc: Russell King 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/arm/mach-mmp/clock.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-mmp/clock.c b/arch/arm/mach-mmp/clock.c
index 28fe64c6e2f5..bdfb113431ec 100644
--- a/arch/arm/mach-mmp/clock.c
+++ b/arch/arm/mach-mmp/clock.c
@@ -83,7 +83,9 @@ unsigned long clk_get_rate(struct clk *clk)
 {
unsigned long rate;
 
-   if (clk->ops->getrate)
+   if (!clk)
+   rate = 0;
+   else if (clk->ops->getrate)
rate = clk->ops->getrate(clk);
else
rate = clk->rate;
-- 
2.11.0

[PATCH 6/9] MIPS: BCM63XX: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: e7300d04bd08 ("MIPS: BCM63xx: Add support for the Broadcom BCM63xx 
family of SOCs.")
Cc: Ralf Baechle <r...@linux-mips.org>
Cc: Florian Fainelli <f.faine...@gmail.com>
Cc: bcm-kernel-feedback-l...@broadcom.com
Cc: James Hogan <james.ho...@imgtec.com>
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/mips/bcm63xx/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/bcm63xx/clk.c b/arch/mips/bcm63xx/clk.c
index 73626040e4d6..19577f771c1f 100644
--- a/arch/mips/bcm63xx/clk.c
+++ b/arch/mips/bcm63xx/clk.c
@@ -339,6 +339,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 
-- 
2.11.0

[PATCH 0/9] make clk_get_rate implementations behavior more consistent

[Jonas Gorski]

The common clock and several other clock API implementations allow
calling clk_get_rate with a NULL pointer. While not specified as
expected behavior of the API, device drivers have come to rely on that,
causing them to OOPS when run on a platform with a different clock API
implementation.

Fix this by making sure all clk_get_rate implementations handle
NULL clocks instead of OOPSing.

While some custom implementations even allow ERR_PTR()s, I decided
against that as IIRC the usual idea is that errors should be handled and
not silently carried over.

Cc: adi-buildroot-de...@lists.sourceforge.net
Cc: bcm-kernel-feedback-l...@broadcom.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-m...@lists.linux-m68k.org
Cc: linux-m...@linux-mips.org

Jonas Gorski (9):
  ARM: ep93xx: allow NULL clock for clk_get_rate
  ARM: mmp: allow NULL clock for clk_get_rate
  blackfin: bf609: allow NULL clock for clk_get_rate
  m68k: allow NULL clock for clk_get_rate
  MIPS: AR7: allow NULL clock for clk_get_rate
  MIPS: BCM63XX: allow NULL clock for clk_get_rate
  MIPS: Loongson 2F: allow NULL clock for clk_get_rate
  MIPS: ralink: allow NULL clock for clk_get_rate
  unicore32: allow NULL clock for clk_get_rate

 arch/arm/mach-ep93xx/clock.c   | 3 +++
 arch/arm/mach-mmp/clock.c  | 4 +++-
 arch/blackfin/mach-bf609/clock.c   | 2 +-
 arch/m68k/coldfire/clk.c   | 3 +++
 arch/mips/ar7/clock.c  | 3 +++
 arch/mips/bcm63xx/clk.c| 3 +++
 arch/mips/loongson64/lemote-2f/clock.c | 3 +++
 arch/mips/ralink/clk.c | 3 +++
 arch/unicore32/kernel/clock.c  | 3 +++
 9 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 6/9] MIPS: BCM63XX: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: e7300d04bd08 ("MIPS: BCM63xx: Add support for the Broadcom BCM63xx 
family of SOCs.")
Cc: Ralf Baechle 
Cc: Florian Fainelli 
Cc: bcm-kernel-feedback-l...@broadcom.com
Cc: James Hogan 
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/mips/bcm63xx/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/bcm63xx/clk.c b/arch/mips/bcm63xx/clk.c
index 73626040e4d6..19577f771c1f 100644
--- a/arch/mips/bcm63xx/clk.c
+++ b/arch/mips/bcm63xx/clk.c
@@ -339,6 +339,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 
-- 
2.11.0

[PATCH 0/9] make clk_get_rate implementations behavior more consistent

[Jonas Gorski]

The common clock and several other clock API implementations allow
calling clk_get_rate with a NULL pointer. While not specified as
expected behavior of the API, device drivers have come to rely on that,
causing them to OOPS when run on a platform with a different clock API
implementation.

Fix this by making sure all clk_get_rate implementations handle
NULL clocks instead of OOPSing.

While some custom implementations even allow ERR_PTR()s, I decided
against that as IIRC the usual idea is that errors should be handled and
not silently carried over.

Cc: adi-buildroot-de...@lists.sourceforge.net
Cc: bcm-kernel-feedback-l...@broadcom.com
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-m...@lists.linux-m68k.org
Cc: linux-m...@linux-mips.org

Jonas Gorski (9):
  ARM: ep93xx: allow NULL clock for clk_get_rate
  ARM: mmp: allow NULL clock for clk_get_rate
  blackfin: bf609: allow NULL clock for clk_get_rate
  m68k: allow NULL clock for clk_get_rate
  MIPS: AR7: allow NULL clock for clk_get_rate
  MIPS: BCM63XX: allow NULL clock for clk_get_rate
  MIPS: Loongson 2F: allow NULL clock for clk_get_rate
  MIPS: ralink: allow NULL clock for clk_get_rate
  unicore32: allow NULL clock for clk_get_rate

 arch/arm/mach-ep93xx/clock.c   | 3 +++
 arch/arm/mach-mmp/clock.c  | 4 +++-
 arch/blackfin/mach-bf609/clock.c   | 2 +-
 arch/m68k/coldfire/clk.c   | 3 +++
 arch/mips/ar7/clock.c  | 3 +++
 arch/mips/bcm63xx/clk.c| 3 +++
 arch/mips/loongson64/lemote-2f/clock.c | 3 +++
 arch/mips/ralink/clk.c | 3 +++
 arch/unicore32/kernel/clock.c  | 3 +++
 9 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.11.0

[PATCH 5/9] MIPS: AR7: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 780019ddf02f ("MIPS: AR7: Implement clock API")
Cc: Ralf Baechle <r...@linux-mips.org>
Cc: Paul Gortmaker <paul.gortma...@windriver.com>
Cc: James Hogan <james.ho...@imgtec.com>
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/mips/ar7/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/ar7/clock.c b/arch/mips/ar7/clock.c
index dda422a0f36c..0137656107a9 100644
--- a/arch/mips/ar7/clock.c
+++ b/arch/mips/ar7/clock.c
@@ -430,6 +430,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 1/9] ARM: ep93xx: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 1d81eedb8f6c ("[ARM] 3634/1: ep93xx: initial implementation of the clk_* 
API")
Cc: Hartley Sweeten <hswee...@visionengravers.com>
Cc: Alexander Sverdlin <alexander.sverd...@gmail.com>
Cc: Russell King <li...@armlinux.org.uk>
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/arm/mach-ep93xx/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..f0768befafe8 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -316,6 +316,9 @@ static unsigned long get_uart_rate(struct clk *clk)
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
if (clk->get_rate)
return clk->get_rate(clk);
 
-- 
2.11.0

[PATCH 5/9] MIPS: AR7: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 780019ddf02f ("MIPS: AR7: Implement clock API")
Cc: Ralf Baechle 
Cc: Paul Gortmaker 
Cc: James Hogan 
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/mips/ar7/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/ar7/clock.c b/arch/mips/ar7/clock.c
index dda422a0f36c..0137656107a9 100644
--- a/arch/mips/ar7/clock.c
+++ b/arch/mips/ar7/clock.c
@@ -430,6 +430,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 1/9] ARM: ep93xx: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 1d81eedb8f6c ("[ARM] 3634/1: ep93xx: initial implementation of the clk_* 
API")
Cc: Hartley Sweeten 
Cc: Alexander Sverdlin 
Cc: Russell King 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/arm/mach-ep93xx/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..f0768befafe8 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -316,6 +316,9 @@ static unsigned long get_uart_rate(struct clk *clk)
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
if (clk->get_rate)
return clk->get_rate(clk);
 
-- 
2.11.0

[PATCH 8/9] MIPS: ralink: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 3f0a06b0368d ("MIPS: ralink: adds clkdev code")
Cc: John Crispin <j...@phrozen.org>
Cc: Ralf Baechle <r...@linux-mips.org>
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/mips/ralink/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
index eb1c61917eb7..1b7df115eb60 100644
--- a/arch/mips/ralink/clk.c
+++ b/arch/mips/ralink/clk.c
@@ -53,6 +53,9 @@ EXPORT_SYMBOL_GPL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL_GPL(clk_get_rate);
-- 
2.11.0

[PATCH 3/9] blackfin: bf609: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 969003152aa9 ("blackfin: bf60x: add clock support")
Cc: Steven Miao <real...@gmail.com>
Cc: Masahiro Yamada <yamada.masah...@socionext.com>
Cc: Andrew Morton <a...@linux-foundation.org>
Cc: adi-buildroot-de...@lists.sourceforge.net
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/blackfin/mach-bf609/clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/blackfin/mach-bf609/clock.c b/arch/blackfin/mach-bf609/clock.c
index 392a59b9a504..21a0ec18829f 100644
--- a/arch/blackfin/mach-bf609/clock.c
+++ b/arch/blackfin/mach-bf609/clock.c
@@ -109,7 +109,7 @@ EXPORT_SYMBOL(clk_disable);
 unsigned long clk_get_rate(struct clk *clk)
 {
unsigned long ret = 0;
-   if (clk->ops && clk->ops->get_rate)
+   if (clk && clk->ops && clk->ops->get_rate)
ret = clk->ops->get_rate(clk);
return ret;
 }
-- 
2.11.0

[PATCH 8/9] MIPS: ralink: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 3f0a06b0368d ("MIPS: ralink: adds clkdev code")
Cc: John Crispin 
Cc: Ralf Baechle 
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/mips/ralink/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
index eb1c61917eb7..1b7df115eb60 100644
--- a/arch/mips/ralink/clk.c
+++ b/arch/mips/ralink/clk.c
@@ -53,6 +53,9 @@ EXPORT_SYMBOL_GPL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL_GPL(clk_get_rate);
-- 
2.11.0

[PATCH 3/9] blackfin: bf609: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 969003152aa9 ("blackfin: bf60x: add clock support")
Cc: Steven Miao 
Cc: Masahiro Yamada 
Cc: Andrew Morton 
Cc: adi-buildroot-de...@lists.sourceforge.net
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/blackfin/mach-bf609/clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/blackfin/mach-bf609/clock.c b/arch/blackfin/mach-bf609/clock.c
index 392a59b9a504..21a0ec18829f 100644
--- a/arch/blackfin/mach-bf609/clock.c
+++ b/arch/blackfin/mach-bf609/clock.c
@@ -109,7 +109,7 @@ EXPORT_SYMBOL(clk_disable);
 unsigned long clk_get_rate(struct clk *clk)
 {
unsigned long ret = 0;
-   if (clk->ops && clk->ops->get_rate)
+   if (clk && clk->ops && clk->ops->get_rate)
ret = clk->ops->get_rate(clk);
return ret;
 }
-- 
2.11.0

[PATCH 7/9] MIPS: Loongson 2F: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter, as some device
drivers rely on this.

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: f8ede0f700f5 ("MIPS: Loongson 2F: Add CPU frequency scaling support")
Cc: Ralf Baechle <r...@linux-mips.org>
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/mips/loongson64/lemote-2f/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/loongson64/lemote-2f/clock.c 
b/arch/mips/loongson64/lemote-2f/clock.c
index a78fb657068c..8281334df9c8 100644
--- a/arch/mips/loongson64/lemote-2f/clock.c
+++ b/arch/mips/loongson64/lemote-2f/clock.c
@@ -80,6 +80,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return (unsigned long)clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 7/9] MIPS: Loongson 2F: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter, as some device
drivers rely on this.

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: f8ede0f700f5 ("MIPS: Loongson 2F: Add CPU frequency scaling support")
Cc: Ralf Baechle 
Cc: linux-m...@linux-mips.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/mips/loongson64/lemote-2f/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/mips/loongson64/lemote-2f/clock.c 
b/arch/mips/loongson64/lemote-2f/clock.c
index a78fb657068c..8281334df9c8 100644
--- a/arch/mips/loongson64/lemote-2f/clock.c
+++ b/arch/mips/loongson64/lemote-2f/clock.c
@@ -80,6 +80,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return (unsigned long)clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 9/9] unicore32: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 64909882862e ("unicore32 additional architecture files: pm related 
files")
Cc: Guan Xuetao <g...@mprc.pku.edu.cn>
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/unicore32/kernel/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/unicore32/kernel/clock.c b/arch/unicore32/kernel/clock.c
index b1ca775f6f6e..d867f34fdb74 100644
--- a/arch/unicore32/kernel/clock.c
+++ b/arch/unicore32/kernel/clock.c
@@ -92,6 +92,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 9/9] unicore32: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: 64909882862e ("unicore32 additional architecture files: pm related 
files")
Cc: Guan Xuetao 
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/unicore32/kernel/clock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/unicore32/kernel/clock.c b/arch/unicore32/kernel/clock.c
index b1ca775f6f6e..d867f34fdb74 100644
--- a/arch/unicore32/kernel/clock.c
+++ b/arch/unicore32/kernel/clock.c
@@ -92,6 +92,9 @@ EXPORT_SYMBOL(clk_disable);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 4/9] m68k: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: facdf0ed4f59 ("m68knommu: introduce basic clk infrastructure")
Cc: Greg Ungerer <g...@linux-m68k.org>
Cc: Geert Uytterhoeven <ge...@linux-m68k.org>
Cc: linux-m...@lists.linux-m68k.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin <d...@kresin.me>
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
 arch/m68k/coldfire/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/m68k/coldfire/clk.c b/arch/m68k/coldfire/clk.c
index 1e3c7e9193d1..856069a3196d 100644
--- a/arch/m68k/coldfire/clk.c
+++ b/arch/m68k/coldfire/clk.c
@@ -121,6 +121,9 @@ EXPORT_SYMBOL(clk_put);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

[PATCH 4/9] m68k: allow NULL clock for clk_get_rate

[Jonas Gorski]

Make the behaviour of clk_get_rate consistent with common clk's
clk_get_rate by accepting NULL clocks as parameter. Some device
drivers rely on this, and will cause an OOPS otherwise.

Fixes: facdf0ed4f59 ("m68knommu: introduce basic clk infrastructure")
Cc: Greg Ungerer 
Cc: Geert Uytterhoeven 
Cc: linux-m...@lists.linux-m68k.org
Cc: linux-kernel@vger.kernel.org
Reported-by: Mathias Kresin 
Signed-off-by: Jonas Gorski 
---
 arch/m68k/coldfire/clk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/m68k/coldfire/clk.c b/arch/m68k/coldfire/clk.c
index 1e3c7e9193d1..856069a3196d 100644
--- a/arch/m68k/coldfire/clk.c
+++ b/arch/m68k/coldfire/clk.c
@@ -121,6 +121,9 @@ EXPORT_SYMBOL(clk_put);
 
 unsigned long clk_get_rate(struct clk *clk)
 {
+   if (!clk)
+   return 0;
+
return clk->rate;
 }
 EXPORT_SYMBOL(clk_get_rate);
-- 
2.11.0

Re: [PATCH v2 09/10] MIPS: i8042: Probe this device only if it exists

[Jonas Gorski]

Hi,

On 28 June 2017 at 17:47, Aleksandar Markovic
 wrote:
> From: Miodrag Dinic 
>
> ARCH_MIGHT_HAVE_PC_SERIO is selected by default for MIPS platforms.
> As a consequence SERIO_I8042 would be automatically selected for any
> MIPS board which wants to enable input support like keyboard
> (INPUT_KEYBOARD) regardless of i8042 controller existence.
>
> The dependency is as follows :
>
> config ARCH_MIGHT_HAVE_PC_SERIO [=y]
> Defined at drivers/input/serio/Kconfig:19
> Depends on: !UML
> Selected by: MIPS [=y]
>
> config SERIO
> Defined at drivers/input/serio/Kconfig:4
> default y
> Depends on: !UML
> Selected by: KEYBOARD_ATKBD [=y] && !UML && INPUT [=y] &&
>  INPUT_KEYBOARD [=y]
>
> config SERIO_I8042
> Defined at drivers/input/serio/Kconfig:28
> tristate "i8042 PC Keyboard controller"
> default y
> Depends on: !UML && SERIO [=y] && ARCH_MIGHT_HAVE_PC_SERIO [=y]
> Selected by: KEYBOARD_ATKBD [=y] && !UML && INPUT [=y] &&
>  INPUT_KEYBOARD [=y] && ARCH_MIGHT_HAVE_PC_SERIO [=y]
>
> If this driver probes the I8042_DATA_REG not knowing if the device
> exists it can cause a kernel to crash. Using check_legacy_ioport()
> interface we can selectively enable this driver only for the MIPS
> boards which actually have the i8042 controller.
>
> New "Ranchu" virtual platform does not support i8042 controller
> so it's added to the blacklist match table.
>
> Each MIPS machine should update this table with it's compatible strings
> if it does not support i8042 controller.
>
> In order to utilize this mechanism, each MIPS machine that do not
> have i8042 controller should update the blacklist table with its
> compatible strings.
>
> Signed-off-by: Miodrag Dinic 
> Signed-off-by: Goran Ferenc 
> Signed-off-by: Aleksandar Markovic 
> ---
>  arch/mips/kernel/setup.c   | 16 
>  drivers/input/serio/i8042-io.h |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index c22cde8..c3e0d2b 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -79,6 +79,15 @@ const unsigned long mips_io_port_base = -1;
>  EXPORT_SYMBOL(mips_io_port_base);
>
>  /*
> + * Here we blacklist all MIPS boards which do not have i8042 controller
> + */
> +static const struct of_device_id i8042_blacklist_of_match[] = {
> +   { .compatible = "mti,ranchu", },
> +   {},
> +};
> +#define I8042_DATA_REG 0x60
> +
> +/*
>   * Check for existence of legacy devices
>   *
>   * Some drivers may try to probe some I/O ports which can lead to
> @@ -90,9 +99,16 @@ EXPORT_SYMBOL(mips_io_port_base);
>   */
>  int check_legacy_ioport(unsigned long base_port)
>  {
> +   struct device_node *np;
> int ret = 0;
>
> switch (base_port) {
> +   case I8042_DATA_REG:
> +   np = of_find_matching_node(NULL, i8042_blacklist_of_match);
> +   if (np)
> +   ret = -ENODEV;
> +   of_node_put(np);
> +   break;

Wouldn't it make more sense to require boards to describe their i8042
device(s) in device tree if USE_OF is enabled? And maybe a whitelist
for those preexisting ones that don't. Much less maintenance overhead
for new boards, as I suspect the amount of boards with i8042 support
is much less than those that don't.

At least PowerPC seems to do it this way.


Regards
Jonas

Re: [PATCH v2 09/10] MIPS: i8042: Probe this device only if it exists

[Jonas Gorski]

Hi,

On 28 June 2017 at 17:47, Aleksandar Markovic
 wrote:
> From: Miodrag Dinic 
>
> ARCH_MIGHT_HAVE_PC_SERIO is selected by default for MIPS platforms.
> As a consequence SERIO_I8042 would be automatically selected for any
> MIPS board which wants to enable input support like keyboard
> (INPUT_KEYBOARD) regardless of i8042 controller existence.
>
> The dependency is as follows :
>
> config ARCH_MIGHT_HAVE_PC_SERIO [=y]
> Defined at drivers/input/serio/Kconfig:19
> Depends on: !UML
> Selected by: MIPS [=y]
>
> config SERIO
> Defined at drivers/input/serio/Kconfig:4
> default y
> Depends on: !UML
> Selected by: KEYBOARD_ATKBD [=y] && !UML && INPUT [=y] &&
>  INPUT_KEYBOARD [=y]
>
> config SERIO_I8042
> Defined at drivers/input/serio/Kconfig:28
> tristate "i8042 PC Keyboard controller"
> default y
> Depends on: !UML && SERIO [=y] && ARCH_MIGHT_HAVE_PC_SERIO [=y]
> Selected by: KEYBOARD_ATKBD [=y] && !UML && INPUT [=y] &&
>  INPUT_KEYBOARD [=y] && ARCH_MIGHT_HAVE_PC_SERIO [=y]
>
> If this driver probes the I8042_DATA_REG not knowing if the device
> exists it can cause a kernel to crash. Using check_legacy_ioport()
> interface we can selectively enable this driver only for the MIPS
> boards which actually have the i8042 controller.
>
> New "Ranchu" virtual platform does not support i8042 controller
> so it's added to the blacklist match table.
>
> Each MIPS machine should update this table with it's compatible strings
> if it does not support i8042 controller.
>
> In order to utilize this mechanism, each MIPS machine that do not
> have i8042 controller should update the blacklist table with its
> compatible strings.
>
> Signed-off-by: Miodrag Dinic 
> Signed-off-by: Goran Ferenc 
> Signed-off-by: Aleksandar Markovic 
> ---
>  arch/mips/kernel/setup.c   | 16 
>  drivers/input/serio/i8042-io.h |  2 +-
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/arch/mips/kernel/setup.c b/arch/mips/kernel/setup.c
> index c22cde8..c3e0d2b 100644
> --- a/arch/mips/kernel/setup.c
> +++ b/arch/mips/kernel/setup.c
> @@ -79,6 +79,15 @@ const unsigned long mips_io_port_base = -1;
>  EXPORT_SYMBOL(mips_io_port_base);
>
>  /*
> + * Here we blacklist all MIPS boards which do not have i8042 controller
> + */
> +static const struct of_device_id i8042_blacklist_of_match[] = {
> +   { .compatible = "mti,ranchu", },
> +   {},
> +};
> +#define I8042_DATA_REG 0x60
> +
> +/*
>   * Check for existence of legacy devices
>   *
>   * Some drivers may try to probe some I/O ports which can lead to
> @@ -90,9 +99,16 @@ EXPORT_SYMBOL(mips_io_port_base);
>   */
>  int check_legacy_ioport(unsigned long base_port)
>  {
> +   struct device_node *np;
> int ret = 0;
>
> switch (base_port) {
> +   case I8042_DATA_REG:
> +   np = of_find_matching_node(NULL, i8042_blacklist_of_match);
> +   if (np)
> +   ret = -ENODEV;
> +   of_node_put(np);
> +   break;

Wouldn't it make more sense to require boards to describe their i8042
device(s) in device tree if USE_OF is enabled? And maybe a whitelist
for those preexisting ones that don't. Much less maintenance overhead
for new boards, as I suspect the amount of boards with i8042 support
is much less than those that don't.

At least PowerPC seems to do it this way.


Regards
Jonas

Re: [PATCH] MIPS: fix boot with DT passed via UHI

[Jonas Gorski]

On 8 June 2017 at 00:47, David Daney <dda...@caviumnetworks.com> wrote:
> On 06/07/2017 06:16 AM, Daniel Schwierzeck wrote:
>>
>>
>>
>> Am 06.06.2017 um 21:16 schrieb Andrea Merello:
>>>
>>> commit 15f37e158892 ("MIPS: store the appended dtb address in a
>>> variable")
>>> seems to have introduced code that relies on delay slots after branch,
>>> however it seems that, since no directive ".set noreorder" is present,
>>> the
>>> AS already fills delay slots with NOPs.
>>>
>>> This caused failure in assigning proper DT blob address to fw_passed_dtb
>>> variable, causing failure when booting passing DT via UHI; this has been
>>> seen on a Lantiq VR9 SoC (Fritzbox 3370) and u-boot as bootloader.
>>>
>>> [0.00] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version
>>> 4.9.0 (GCC) ) #29 SMP Tue Jun 6 20:49:59 CEST 2017
>>> [0.00] SoC: xRX200 rev 1.2
>>> [0.00] bootconsole [early0] enabled
>>> [0.00] CPU0 revision is: 00019556 (MIPS 34Kc)
>>> [0.00] Determined physical RAM map:
>>> [0.00]  memory: 00696000 @ 2000 (usable)
>>> [0.00]  memory: 00038000 @ 00698000 (usable after init)
>>> [0.00] Wasting 64 bytes for tracking 2 unused pages
>>> [0.00] Kernel panic - not syncing: No memory area to place a
>>> bootmap bitmap
>>> [0.00] Rebooting in 1 seconds..
>>> [0.00] Reboot failed -- System halted
>>>
>>> This patch moves the instruction meant to be placed in the delay slot
>>> before the preceding BEQ instruction, while the delay slot will be
>>> filled with a NOP by the AS.
>>>
>>> After this patch the kernel fetches the DR correctly
>>>
>>> [0.00] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version
>>> 4.9.0 (GCC) ) #30 SMP
>>> Tue Jun 6 20:52:40 CEST 2017
>>> [0.00] SoC: xRX200 rev 1.2
>>> [0.00] bootconsole [early0] enabled
>>> [0.00] CPU0 revision is: 00019556 (MIPS 34Kc)
>>> [0.00] MIPS: machine is FRITZ3370 - Fritz!Box WLAN 3370
>>> [0.00] Determined physical RAM map:
>>> [0.00]  memory: 0800 @  (usable)
>>> [0.00] Detected 1 available secondary CPU(s)
>>> [0.00] Primary instruction cache 32kB, VIPT, 4-way, linesize 32
>>> bytes.
>>> [0.00] Primary data cache 32kB, 4-way, VIPT, cache aliases,
>>> linesize 32 bytes
>>> [0.00] Zone ranges:
>>> [0.00]   Normal   [mem 0x-0x07ff]
>>> [0.00] Movable zone start for each node
>>> [0.00] Early memory node ranges
>>> [0.00]   node   0: [mem 0x-0x07ff]
>>> [0.00] Initmem setup node 0 [mem
>>> 0x-0x07ff]
>>> [0.00] percpu: Embedded 15 pages/cpu @8110c000 s30176 r8192
>>> d23072 u61440
>>> [0.00] Built 1 zonelists in Zone order, mobility grouping on.
>>> Total pages: 32512
>>> [0.00] Kernel command line: rootwait root=/dev/sda1
>>> console=ttyLTQ0
>>> ...
>>>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: Jonas Gorski <j...@openwrt.org>
>>> Cc: Daniel Schwierzeck <daniel.schwierz...@gmail.com>
>>> Signed-off-by: Andrea Merello <andrea.mere...@gmail.com>
>>>
>>> diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
>>> index cf05220..d1bb506 100644
>>> --- a/arch/mips/kernel/head.S
>>> +++ b/arch/mips/kernel/head.S
>>> @@ -106,8 +106,8 @@ NESTED(kernel_entry, 16, sp)#
>>> kernel entry point
>>> beq t0, t1, dtb_found
>>>   #endif
>>> li  t1, -2
>>> -   beq a0, t1, dtb_found
>>> movet2, a1
>>> +   beq a0, t1, dtb_found
>>> li  t2, 0
>>>   dtb_found:
>>>
>>
>> The fix looks correct. Without ".set noreorder" one should not
>
>
> s/should/must/
>
>> manually
>> put instructions in the delay slot. This should be left to the AS as an
>> option for optimization.
>
>
> By definition, it is what the assembler does.  When ".set noreorder" is not
> in effect, the source code *must* be written as if branch delay slots do not
> exist.  There is no option here.

My assembly knowledge is mostly from reading disassembled code where
delay slots always matter, so when writing this code it didn't even
cross my mind that the the assembler might do that.

So thanks for fixing it!


Regards
Jonas

Re: [PATCH] MIPS: fix boot with DT passed via UHI

[Jonas Gorski]

On 8 June 2017 at 00:47, David Daney  wrote:
> On 06/07/2017 06:16 AM, Daniel Schwierzeck wrote:
>>
>>
>>
>> Am 06.06.2017 um 21:16 schrieb Andrea Merello:
>>>
>>> commit 15f37e158892 ("MIPS: store the appended dtb address in a
>>> variable")
>>> seems to have introduced code that relies on delay slots after branch,
>>> however it seems that, since no directive ".set noreorder" is present,
>>> the
>>> AS already fills delay slots with NOPs.
>>>
>>> This caused failure in assigning proper DT blob address to fw_passed_dtb
>>> variable, causing failure when booting passing DT via UHI; this has been
>>> seen on a Lantiq VR9 SoC (Fritzbox 3370) and u-boot as bootloader.
>>>
>>> [0.00] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version
>>> 4.9.0 (GCC) ) #29 SMP Tue Jun 6 20:49:59 CEST 2017
>>> [0.00] SoC: xRX200 rev 1.2
>>> [0.00] bootconsole [early0] enabled
>>> [0.00] CPU0 revision is: 00019556 (MIPS 34Kc)
>>> [0.00] Determined physical RAM map:
>>> [0.00]  memory: 00696000 @ 2000 (usable)
>>> [0.00]  memory: 00038000 @ 00698000 (usable after init)
>>> [0.00] Wasting 64 bytes for tracking 2 unused pages
>>> [0.00] Kernel panic - not syncing: No memory area to place a
>>> bootmap bitmap
>>> [0.00] Rebooting in 1 seconds..
>>> [0.00] Reboot failed -- System halted
>>>
>>> This patch moves the instruction meant to be placed in the delay slot
>>> before the preceding BEQ instruction, while the delay slot will be
>>> filled with a NOP by the AS.
>>>
>>> After this patch the kernel fetches the DR correctly
>>>
>>> [0.00] Linux version 4.12.0-fritz+ (andrea@horizon) (gcc version
>>> 4.9.0 (GCC) ) #30 SMP
>>> Tue Jun 6 20:52:40 CEST 2017
>>> [0.00] SoC: xRX200 rev 1.2
>>> [0.00] bootconsole [early0] enabled
>>> [0.00] CPU0 revision is: 00019556 (MIPS 34Kc)
>>> [0.00] MIPS: machine is FRITZ3370 - Fritz!Box WLAN 3370
>>> [0.00] Determined physical RAM map:
>>> [0.00]  memory: 0800 @  (usable)
>>> [0.00] Detected 1 available secondary CPU(s)
>>> [0.00] Primary instruction cache 32kB, VIPT, 4-way, linesize 32
>>> bytes.
>>> [0.00] Primary data cache 32kB, 4-way, VIPT, cache aliases,
>>> linesize 32 bytes
>>> [0.00] Zone ranges:
>>> [0.00]   Normal   [mem 0x-0x07ff]
>>> [0.00] Movable zone start for each node
>>> [0.00] Early memory node ranges
>>> [0.00]   node   0: [mem 0x-0x07ff]
>>> [0.00] Initmem setup node 0 [mem
>>> 0x-0x07ff]
>>> [0.00] percpu: Embedded 15 pages/cpu @8110c000 s30176 r8192
>>> d23072 u61440
>>> [0.00] Built 1 zonelists in Zone order, mobility grouping on.
>>> Total pages: 32512
>>> [0.00] Kernel command line: rootwait root=/dev/sda1
>>> console=ttyLTQ0
>>> ...
>>>
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: Jonas Gorski 
>>> Cc: Daniel Schwierzeck 
>>> Signed-off-by: Andrea Merello 
>>>
>>> diff --git a/arch/mips/kernel/head.S b/arch/mips/kernel/head.S
>>> index cf05220..d1bb506 100644
>>> --- a/arch/mips/kernel/head.S
>>> +++ b/arch/mips/kernel/head.S
>>> @@ -106,8 +106,8 @@ NESTED(kernel_entry, 16, sp)#
>>> kernel entry point
>>> beq t0, t1, dtb_found
>>>   #endif
>>> li  t1, -2
>>> -   beq a0, t1, dtb_found
>>> movet2, a1
>>> +   beq a0, t1, dtb_found
>>> li  t2, 0
>>>   dtb_found:
>>>
>>
>> The fix looks correct. Without ".set noreorder" one should not
>
>
> s/should/must/
>
>> manually
>> put instructions in the delay slot. This should be left to the AS as an
>> option for optimization.
>
>
> By definition, it is what the assembler does.  When ".set noreorder" is not
> in effect, the source code *must* be written as if branch delay slots do not
> exist.  There is no option here.

My assembly knowledge is mostly from reading disassembled code where
delay slots always matter, so when writing this code it didn't even
cross my mind that the the assembler might do that.

So thanks for fixing it!


Regards
Jonas

Re: [PATCH] regmap: make LZO cache optional

[Jonas Gorski]

Hi Andreas,

On 8 June 2017 at 16:28, Andreas Ziegler  wrote:
> Hi Jonas,
>
> I noticed your patch 'regmap: make LZO cache optional' as it recently showed 
> up
> in linux-next. In your patch, you modify drivers/base/regmap/regcache.c by
> adding an #if IS_ENABLED() statement.
>
> However, this statement contains a spelling error, as it references
> REGCHACHE_COMPRESSED instead of REGCACHE_COMPRESSED (note the extra H).
>
> I noticed it by running the in-tree script at scripts/checkkconfigsymbols.py 
> on
> the commit, like so: './scripts/checkkconfigsymbols.py -c 34a730aa74c7'
>
> As Greg suggested the whole code could be dropped, this might not be too
> relevant, but I wanted to let you know in any case.

Thanks for spotting that, and I wasn't aware of this script, that
seems quite useful!


Regards
Jonas

Re: [PATCH] regmap: make LZO cache optional

[Jonas Gorski]

Hi Andreas,

On 8 June 2017 at 16:28, Andreas Ziegler  wrote:
> Hi Jonas,
>
> I noticed your patch 'regmap: make LZO cache optional' as it recently showed 
> up
> in linux-next. In your patch, you modify drivers/base/regmap/regcache.c by
> adding an #if IS_ENABLED() statement.
>
> However, this statement contains a spelling error, as it references
> REGCHACHE_COMPRESSED instead of REGCACHE_COMPRESSED (note the extra H).
>
> I noticed it by running the in-tree script at scripts/checkkconfigsymbols.py 
> on
> the commit, like so: './scripts/checkkconfigsymbols.py -c 34a730aa74c7'
>
> As Greg suggested the whole code could be dropped, this might not be too
> relevant, but I wanted to let you know in any case.

Thanks for spotting that, and I wasn't aware of this script, that
seems quite useful!


Regards
Jonas

[PATCH V2] regmap: drop LZO cache support

[Jonas Gorski]

This is effectivly a revert of 2cbbb579bcbe3 ("regmap: Add the LZO cache
support").

After it was added there were never any users added, the only submitted
user was changed to RBTREE before being added. It has been unused code
for almost six years. Since LZO support itself has its own size, it
currently is rather a deoptimization.

Saves ~46 kB on MIPS (size of LZO support + regcache LZO code).

Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
V1 -> V2:
 * remove code instead of hide code behind Kconfig option

 drivers/base/regmap/Kconfig|   2 -
 drivers/base/regmap/Makefile   |   2 +-
 drivers/base/regmap/internal.h |   1 -
 drivers/base/regmap/regcache-lzo.c | 374 -
 drivers/base/regmap/regcache.c |   1 -
 include/linux/regmap.h |   1 -
 6 files changed, 1 insertion(+), 380 deletions(-)
 delete mode 100644 drivers/base/regmap/regcache-lzo.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c36a3e..32b1ee91d1b4 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,8 +4,6 @@
 
 config REGMAP
default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || 
REGMAP_MMIO || REGMAP_IRQ)
-   select LZO_COMPRESS
-   select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
bool
 
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c84f485..0ba68a269805 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,7 +2,7 @@
 CFLAGS_regmap.o := -I$(src)
 
 obj-$(CONFIG_REGMAP) += regmap.o regcache.o
-obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
+obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-flat.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_AC97) += regmap-ac97.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 2a4435d76028..c7cd47bd797d 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -255,7 +255,6 @@ enum regmap_endian regmap_get_val_endian(struct device *dev,
 const struct regmap_config *config);
 
 extern struct regcache_ops regcache_rbtree_ops;
-extern struct regcache_ops regcache_lzo_ops;
 extern struct regcache_ops regcache_flat_ops;
 
 static inline const char *regmap_name(const struct regmap *map)
diff --git a/drivers/base/regmap/regcache-lzo.c 
b/drivers/base/regmap/regcache-lzo.c
deleted file mode 100644
index 4ff311374c4a..
--- a/drivers/base/regmap/regcache-lzo.c
+++ /dev/null
@@ -1,374 +0,0 @@
-/*
- * Register cache access API - LZO caching support
- *
- * Copyright 2011 Wolfson Microelectronics plc
- *
- * Author: Dimitris Papastamos <d...@opensource.wolfsonmicro.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include 
-#include 
-#include 
-
-#include "internal.h"
-
-static int regcache_lzo_exit(struct regmap *map);
-
-struct regcache_lzo_ctx {
-   void *wmem;
-   void *dst;
-   const void *src;
-   size_t src_len;
-   size_t dst_len;
-   size_t decompressed_size;
-   unsigned long *sync_bmp;
-   int sync_bmp_nbits;
-};
-
-#define LZO_BLOCK_NUM 8
-static int regcache_lzo_block_count(struct regmap *map)
-{
-   return LZO_BLOCK_NUM;
-}
-
-static int regcache_lzo_prepare(struct regcache_lzo_ctx *lzo_ctx)
-{
-   lzo_ctx->wmem = kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
-   if (!lzo_ctx->wmem)
-   return -ENOMEM;
-   return 0;
-}
-
-static int regcache_lzo_compress(struct regcache_lzo_ctx *lzo_ctx)
-{
-   size_t compress_size;
-   int ret;
-
-   ret = lzo1x_1_compress(lzo_ctx->src, lzo_ctx->src_len,
-  lzo_ctx->dst, _size, lzo_ctx->wmem);
-   if (ret != LZO_E_OK || compress_size > lzo_ctx->dst_len)
-   return -EINVAL;
-   lzo_ctx->dst_len = compress_size;
-   return 0;
-}
-
-static int regcache_lzo_decompress(struct regcache_lzo_ctx *lzo_ctx)
-{
-   size_t dst_len;
-   int ret;
-
-   dst_len = lzo_ctx->dst_len;
-   ret = lzo1x_decompress_safe(lzo_ctx->src, lzo_ctx->src_len,
-   lzo_ctx->dst, _len);
-   if (ret != LZO_E_OK || dst_len != lzo_ctx->dst_len)
-   return -EINVAL;
-   return 0;
-}
-
-static int regcache_lzo_compress_cache_block(struct regmap *map,
-   struct regcache_lzo_ctx *lzo_ctx)
-{
-   int ret;
-
-   lzo_ctx->dst_len = lzo1x_worst_compress(PAGE_SIZE);
-   lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL);
-   if (!lzo_ctx->dst) {
-   lzo_ctx->dst_len = 0;
-   r

[PATCH V2] regmap: drop LZO cache support

[Jonas Gorski]

This is effectivly a revert of 2cbbb579bcbe3 ("regmap: Add the LZO cache
support").

After it was added there were never any users added, the only submitted
user was changed to RBTREE before being added. It has been unused code
for almost six years. Since LZO support itself has its own size, it
currently is rather a deoptimization.

Saves ~46 kB on MIPS (size of LZO support + regcache LZO code).

Signed-off-by: Jonas Gorski 
---
V1 -> V2:
 * remove code instead of hide code behind Kconfig option

 drivers/base/regmap/Kconfig|   2 -
 drivers/base/regmap/Makefile   |   2 +-
 drivers/base/regmap/internal.h |   1 -
 drivers/base/regmap/regcache-lzo.c | 374 -
 drivers/base/regmap/regcache.c |   1 -
 include/linux/regmap.h |   1 -
 6 files changed, 1 insertion(+), 380 deletions(-)
 delete mode 100644 drivers/base/regmap/regcache-lzo.c

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c36a3e..32b1ee91d1b4 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,8 +4,6 @@
 
 config REGMAP
default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || 
REGMAP_MMIO || REGMAP_IRQ)
-   select LZO_COMPRESS
-   select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
bool
 
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c84f485..0ba68a269805 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,7 +2,7 @@
 CFLAGS_regmap.o := -I$(src)
 
 obj-$(CONFIG_REGMAP) += regmap.o regcache.o
-obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
+obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-flat.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_AC97) += regmap-ac97.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 2a4435d76028..c7cd47bd797d 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -255,7 +255,6 @@ enum regmap_endian regmap_get_val_endian(struct device *dev,
 const struct regmap_config *config);
 
 extern struct regcache_ops regcache_rbtree_ops;
-extern struct regcache_ops regcache_lzo_ops;
 extern struct regcache_ops regcache_flat_ops;
 
 static inline const char *regmap_name(const struct regmap *map)
diff --git a/drivers/base/regmap/regcache-lzo.c 
b/drivers/base/regmap/regcache-lzo.c
deleted file mode 100644
index 4ff311374c4a..
--- a/drivers/base/regmap/regcache-lzo.c
+++ /dev/null
@@ -1,374 +0,0 @@
-/*
- * Register cache access API - LZO caching support
- *
- * Copyright 2011 Wolfson Microelectronics plc
- *
- * Author: Dimitris Papastamos 
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include 
-#include 
-#include 
-
-#include "internal.h"
-
-static int regcache_lzo_exit(struct regmap *map);
-
-struct regcache_lzo_ctx {
-   void *wmem;
-   void *dst;
-   const void *src;
-   size_t src_len;
-   size_t dst_len;
-   size_t decompressed_size;
-   unsigned long *sync_bmp;
-   int sync_bmp_nbits;
-};
-
-#define LZO_BLOCK_NUM 8
-static int regcache_lzo_block_count(struct regmap *map)
-{
-   return LZO_BLOCK_NUM;
-}
-
-static int regcache_lzo_prepare(struct regcache_lzo_ctx *lzo_ctx)
-{
-   lzo_ctx->wmem = kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
-   if (!lzo_ctx->wmem)
-   return -ENOMEM;
-   return 0;
-}
-
-static int regcache_lzo_compress(struct regcache_lzo_ctx *lzo_ctx)
-{
-   size_t compress_size;
-   int ret;
-
-   ret = lzo1x_1_compress(lzo_ctx->src, lzo_ctx->src_len,
-  lzo_ctx->dst, _size, lzo_ctx->wmem);
-   if (ret != LZO_E_OK || compress_size > lzo_ctx->dst_len)
-   return -EINVAL;
-   lzo_ctx->dst_len = compress_size;
-   return 0;
-}
-
-static int regcache_lzo_decompress(struct regcache_lzo_ctx *lzo_ctx)
-{
-   size_t dst_len;
-   int ret;
-
-   dst_len = lzo_ctx->dst_len;
-   ret = lzo1x_decompress_safe(lzo_ctx->src, lzo_ctx->src_len,
-   lzo_ctx->dst, _len);
-   if (ret != LZO_E_OK || dst_len != lzo_ctx->dst_len)
-   return -EINVAL;
-   return 0;
-}
-
-static int regcache_lzo_compress_cache_block(struct regmap *map,
-   struct regcache_lzo_ctx *lzo_ctx)
-{
-   int ret;
-
-   lzo_ctx->dst_len = lzo1x_worst_compress(PAGE_SIZE);
-   lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL);
-   if (!lzo_ctx->dst) {
-   lzo_ctx->dst_len = 0;
-   return -ENOMEM;
-   }
-
-   ret = regcache_lzo_compress(lzo_ctx);

[PATCH] regmap: make LZO cache optional

[Jonas Gorski]

Commit 2cbbb579bcbe3 ("regmap: Add the LZO cache support") added support
for LZO compression in regcache, but there were never any users added
afterwards. Since LZO support itself has its own size, it currently is
rather a deoptimization.

So make it optional by introducing a symbol that can be selected by
drivers wanting to make use of it.

Saves e.g. ~46 kB on MIPS (size of LZO support + regcache LZO code).

Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
I tried using google to find any users (even out-of-tree ones), but at
best I found a single driver submission that was switched to RBTREE in
subsequent resubmissions (MFD_SMSC).

One could maybe also just drop the code because of no users for 5 years,
but that would be up to the maintainer(s) to decide.

 drivers/base/regmap/Kconfig| 5 -
 drivers/base/regmap/Makefile   | 3 ++-
 drivers/base/regmap/regcache.c | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c36a3e..48b3fc1ee514 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,9 +4,12 @@
 
 config REGMAP
default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || 
REGMAP_MMIO || REGMAP_IRQ)
+   select IRQ_DOMAIN if REGMAP_IRQ
+   bool
+
+config REGCACHE_COMPRESSED
select LZO_COMPRESS
select LZO_DECOMPRESS
-   select IRQ_DOMAIN if REGMAP_IRQ
bool
 
 config REGMAP_AC97
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c84f485..6271ea9b758a 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,7 +2,8 @@
 CFLAGS_regmap.o := -I$(src)
 
 obj-$(CONFIG_REGMAP) += regmap.o regcache.o
-obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
+obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-flat.o
+obj-$(CONFIG_REGCACHE_COMPRESSED) += regcache-lzo.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_AC97) += regmap-ac97.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index b0a0dcf32fb7..f3a435ee5fe8 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -21,7 +21,9 @@
 
 static const struct regcache_ops *cache_types[] = {
_rbtree_ops,
+#if IS_ENABLED(CONFIG_REGCHACHE_COMPRESSED)
_lzo_ops,
+#endif
_flat_ops,
 };
 
-- 
2.11.0

[PATCH] regmap: make LZO cache optional

[Jonas Gorski]

Commit 2cbbb579bcbe3 ("regmap: Add the LZO cache support") added support
for LZO compression in regcache, but there were never any users added
afterwards. Since LZO support itself has its own size, it currently is
rather a deoptimization.

So make it optional by introducing a symbol that can be selected by
drivers wanting to make use of it.

Saves e.g. ~46 kB on MIPS (size of LZO support + regcache LZO code).

Signed-off-by: Jonas Gorski 
---
I tried using google to find any users (even out-of-tree ones), but at
best I found a single driver submission that was switched to RBTREE in
subsequent resubmissions (MFD_SMSC).

One could maybe also just drop the code because of no users for 5 years,
but that would be up to the maintainer(s) to decide.

 drivers/base/regmap/Kconfig| 5 -
 drivers/base/regmap/Makefile   | 3 ++-
 drivers/base/regmap/regcache.c | 2 ++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index db9d00c36a3e..48b3fc1ee514 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -4,9 +4,12 @@
 
 config REGMAP
default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_AC97 || 
REGMAP_MMIO || REGMAP_IRQ)
+   select IRQ_DOMAIN if REGMAP_IRQ
+   bool
+
+config REGCACHE_COMPRESSED
select LZO_COMPRESS
select LZO_DECOMPRESS
-   select IRQ_DOMAIN if REGMAP_IRQ
bool
 
 config REGMAP_AC97
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 609e4c84f485..6271ea9b758a 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -2,7 +2,8 @@
 CFLAGS_regmap.o := -I$(src)
 
 obj-$(CONFIG_REGMAP) += regmap.o regcache.o
-obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
+obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-flat.o
+obj-$(CONFIG_REGCACHE_COMPRESSED) += regcache-lzo.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_AC97) += regmap-ac97.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index b0a0dcf32fb7..f3a435ee5fe8 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -21,7 +21,9 @@
 
 static const struct regcache_ops *cache_types[] = {
_rbtree_ops,
+#if IS_ENABLED(CONFIG_REGCHACHE_COMPRESSED)
_lzo_ops,
+#endif
_flat_ops,
 };
 
-- 
2.11.0

Re: [RESEND PATCH] spi: bcm63xx-hsspi: Export OF device ID table as module aliases

[Jonas Gorski]

On 1 May 2017 at 22:13, Andres Galacho <andresgala...@gmail.com> wrote:
> The device table is required to load modules based on
> modaliases. After adding MODULE_DEVICE_TABLE, below entries
> for example will be added to module.alias:
> alias:  of:N*T*Cbrcm,bcm6328-hsspiC*
> alias:  of:N*T*Cbrcm,bcm6328-hsspi
>
> Signed-off-by: Andres Galacho <andresgala...@gmail.com>

Not that it matters much with a trivial patch like this, but

Acked-by: Jonas Gorski <jonas.gor...@gmail.com>

also thanks for doing it.


Regards
Jonas

Re: [RESEND PATCH] spi: bcm63xx-hsspi: Export OF device ID table as module aliases

[Jonas Gorski]

On 1 May 2017 at 22:13, Andres Galacho  wrote:
> The device table is required to load modules based on
> modaliases. After adding MODULE_DEVICE_TABLE, below entries
> for example will be added to module.alias:
> alias:  of:N*T*Cbrcm,bcm6328-hsspiC*
> alias:  of:N*T*Cbrcm,bcm6328-hsspi
>
> Signed-off-by: Andres Galacho 

Not that it matters much with a trivial patch like this, but

Acked-by: Jonas Gorski 

also thanks for doing it.


Regards
Jonas

Re: [PATCH] MIPS: Allow compressed images to be loaded at the usual address

[Jonas Gorski]

Hi,

On 13 February 2017 at 09:37, Alban <al...@free.fr> wrote:
> On Thu, 9 Feb 2017 13:22:37 +0100
> Jonas Gorski <jonas.gor...@gmail.com> wrote:
>
>> Hi,
>>
>> On 5 February 2017 at 21:21, Alban <al...@free.fr> wrote:
>> > From: Alban Bedel <al...@free.fr>
>> >
>> > Normally compressed images have to be loaded at a different address to
>> > allow the decompressor to run. This add an option to let vmlinuz copy
>> > itself to the correct address from the normal vmlinux address.
>> >
>> > Signed-off-by: Alban Bedel <al...@free.fr>
>> > ---
>> >  arch/mips/Kconfig|  8 
>> >  arch/mips/boot/compressed/head.S | 13 +
>> >  2 files changed, 21 insertions(+)
>> >
>> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>> > index b3c5bde..8074fc5 100644
>> > --- a/arch/mips/Kconfig
>> > +++ b/arch/mips/Kconfig
>> > @@ -2961,6 +2961,14 @@ choice
>> > bool "Extend builtin kernel arguments with bootloader 
>> > arguments"
>> >  endchoice
>> >
>> > +config ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
>> > +   bool "Load compressed images at the same address as uncompressed"
>> > +   depends on SYS_SUPPORTS_ZBOOT
>> > +   help
>> > + vmlinux and vmlinuz normally have different load addresses, with
>> > + this option vmlinuz expect to be loaded at the same address as
>> > + vmlinux.
>> > +
>> >  endmenu
>>
>> Okay, it took me a while to understand the intention of this change. I
>> thought it was for supporting the case that VMLINUZ_LOAD_ADDRESS ==
>> VMLINUX_LOAD_ADDRESS, but it is indented for  VMLINUZ_LOAD_ADDRESS !=
>> VMLINUX_LOAD_ADDRESS, but still being loaded at VMLINUX_LOAD_ADDRESS.
>>
>> So I guess that this can only happen with vmlinuz.bin, as vmlinux's
>> ELF header will cause it to be loaded at the expected address (for
>> sane bootloaders at least).
>
> Yes, this is for bootloaders that use raw images. Having to configure
> different load addresses for compressed and uncompressed images was just
> too annoying.
>
>> >  config LOCKDEP_SUPPORT
>> > diff --git a/arch/mips/boot/compressed/head.S 
>> > b/arch/mips/boot/compressed/head.S
>> > index 409cb48..a215171 100644
>> > --- a/arch/mips/boot/compressed/head.S
>> > +++ b/arch/mips/boot/compressed/head.S
>> > @@ -25,6 +25,19 @@ start:
>> > moves2, a2
>> > moves3, a3
>> >
>> > +#ifdef CONFIG_ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
>>
>> With a bit of BAL trickery you could easily detect this at runtime and
>> then conditionally copy without requiring any additional config
>> symbols. Then you aren't limited to being executed from
>> VMLINUX_LOAD_ADDRESS.
>
> Could you expand a bit on what you mean with "BAL trickery"? I hoped
> that it would be possible to auto detect the current running address,
> but as I know very little about MIPS assembly I didn't found how that
> could be done.

With BAL (branch and link) you can do a pc-relative jump, and the
current address will be stored in $ra. comparing it with the expected
address will give you the offset by which you were loaded. To quote
the lzma-loader from OpenWrt/Lede[1]:

la  t0, __reloc_label   # get linked address of label
bal __reloc_label   # branch and link to label to
nop # get actual address
__reloc_label:
subut0, ra, t0  # get reloc_delta
beqzt0, __reloc_done # if delta is 0 we are in the
right place
nop

/* Copy our code to the right place */
la  t1, _code_start # get linked address of _code_start
la  t2, _code_end   # get linked address of _code_end
addut0, t0, t1  # calculate actual address of
_code_start

__reloc_copy:
...
__reloc_done:
...


Regards
Jonas

[1] 
https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/ar71xx/image/lzma-loader/src/head.S;h=47a7c9bd6300ad92e6a0d426c5f44bc0f3e7e85f;hb=HEAD#l49

Re: [PATCH] MIPS: Allow compressed images to be loaded at the usual address

[Jonas Gorski]

Hi,

On 13 February 2017 at 09:37, Alban  wrote:
> On Thu, 9 Feb 2017 13:22:37 +0100
> Jonas Gorski  wrote:
>
>> Hi,
>>
>> On 5 February 2017 at 21:21, Alban  wrote:
>> > From: Alban Bedel 
>> >
>> > Normally compressed images have to be loaded at a different address to
>> > allow the decompressor to run. This add an option to let vmlinuz copy
>> > itself to the correct address from the normal vmlinux address.
>> >
>> > Signed-off-by: Alban Bedel 
>> > ---
>> >  arch/mips/Kconfig|  8 
>> >  arch/mips/boot/compressed/head.S | 13 +
>> >  2 files changed, 21 insertions(+)
>> >
>> > diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
>> > index b3c5bde..8074fc5 100644
>> > --- a/arch/mips/Kconfig
>> > +++ b/arch/mips/Kconfig
>> > @@ -2961,6 +2961,14 @@ choice
>> > bool "Extend builtin kernel arguments with bootloader 
>> > arguments"
>> >  endchoice
>> >
>> > +config ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
>> > +   bool "Load compressed images at the same address as uncompressed"
>> > +   depends on SYS_SUPPORTS_ZBOOT
>> > +   help
>> > + vmlinux and vmlinuz normally have different load addresses, with
>> > + this option vmlinuz expect to be loaded at the same address as
>> > + vmlinux.
>> > +
>> >  endmenu
>>
>> Okay, it took me a while to understand the intention of this change. I
>> thought it was for supporting the case that VMLINUZ_LOAD_ADDRESS ==
>> VMLINUX_LOAD_ADDRESS, but it is indented for  VMLINUZ_LOAD_ADDRESS !=
>> VMLINUX_LOAD_ADDRESS, but still being loaded at VMLINUX_LOAD_ADDRESS.
>>
>> So I guess that this can only happen with vmlinuz.bin, as vmlinux's
>> ELF header will cause it to be loaded at the expected address (for
>> sane bootloaders at least).
>
> Yes, this is for bootloaders that use raw images. Having to configure
> different load addresses for compressed and uncompressed images was just
> too annoying.
>
>> >  config LOCKDEP_SUPPORT
>> > diff --git a/arch/mips/boot/compressed/head.S 
>> > b/arch/mips/boot/compressed/head.S
>> > index 409cb48..a215171 100644
>> > --- a/arch/mips/boot/compressed/head.S
>> > +++ b/arch/mips/boot/compressed/head.S
>> > @@ -25,6 +25,19 @@ start:
>> > moves2, a2
>> > moves3, a3
>> >
>> > +#ifdef CONFIG_ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
>>
>> With a bit of BAL trickery you could easily detect this at runtime and
>> then conditionally copy without requiring any additional config
>> symbols. Then you aren't limited to being executed from
>> VMLINUX_LOAD_ADDRESS.
>
> Could you expand a bit on what you mean with "BAL trickery"? I hoped
> that it would be possible to auto detect the current running address,
> but as I know very little about MIPS assembly I didn't found how that
> could be done.

With BAL (branch and link) you can do a pc-relative jump, and the
current address will be stored in $ra. comparing it with the expected
address will give you the offset by which you were loaded. To quote
the lzma-loader from OpenWrt/Lede[1]:

la  t0, __reloc_label   # get linked address of label
bal __reloc_label   # branch and link to label to
nop # get actual address
__reloc_label:
subut0, ra, t0  # get reloc_delta
beqzt0, __reloc_done # if delta is 0 we are in the
right place
nop

/* Copy our code to the right place */
la  t1, _code_start # get linked address of _code_start
la  t2, _code_end   # get linked address of _code_end
addut0, t0, t1  # calculate actual address of
_code_start

__reloc_copy:
...
__reloc_done:
...


Regards
Jonas

[1] 
https://git.lede-project.org/?p=source.git;a=blob;f=target/linux/ar71xx/image/lzma-loader/src/head.S;h=47a7c9bd6300ad92e6a0d426c5f44bc0f3e7e85f;hb=HEAD#l49

Re: [PATCH] MIPS: Allow compressed images to be loaded at the usual address

[Jonas Gorski]

Hi,

On 5 February 2017 at 21:21, Alban  wrote:
> From: Alban Bedel 
>
> Normally compressed images have to be loaded at a different address to
> allow the decompressor to run. This add an option to let vmlinuz copy
> itself to the correct address from the normal vmlinux address.
>
> Signed-off-by: Alban Bedel 
> ---
>  arch/mips/Kconfig|  8 
>  arch/mips/boot/compressed/head.S | 13 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index b3c5bde..8074fc5 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -2961,6 +2961,14 @@ choice
> bool "Extend builtin kernel arguments with bootloader 
> arguments"
>  endchoice
>
> +config ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
> +   bool "Load compressed images at the same address as uncompressed"
> +   depends on SYS_SUPPORTS_ZBOOT
> +   help
> + vmlinux and vmlinuz normally have different load addresses, with
> + this option vmlinuz expect to be loaded at the same address as
> + vmlinux.
> +
>  endmenu

Okay, it took me a while to understand the intention of this change. I
thought it was for supporting the case that VMLINUZ_LOAD_ADDRESS ==
VMLINUX_LOAD_ADDRESS, but it is indented for  VMLINUZ_LOAD_ADDRESS !=
VMLINUX_LOAD_ADDRESS, but still being loaded at VMLINUX_LOAD_ADDRESS.

So I guess that this can only happen with vmlinuz.bin, as vmlinux's
ELF header will cause it to be loaded at the expected address (for
sane bootloaders at least).

>  config LOCKDEP_SUPPORT
> diff --git a/arch/mips/boot/compressed/head.S 
> b/arch/mips/boot/compressed/head.S
> index 409cb48..a215171 100644
> --- a/arch/mips/boot/compressed/head.S
> +++ b/arch/mips/boot/compressed/head.S
> @@ -25,6 +25,19 @@ start:
> moves2, a2
> moves3, a3
>
> +#ifdef CONFIG_ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS

With a bit of BAL trickery you could easily detect this at runtime and
then conditionally copy without requiring any additional config
symbols. Then you aren't limited to being executed from
VMLINUX_LOAD_ADDRESS.

> +   /* Move the text, data section and DTB to the correct address */
> +   PTR_LA  a0, .text
> +   PTR_LI  a1, VMLINUX_LOAD_ADDRESS
> +   PTR_LA  a2, _edata
> +0:
> +   lw  a3, 0(a1)
> +   sw  a3, 0(a0)
> +   addiu   a1, a1, 4
> +   bne a2, a0, 0b
> +addiu  a0, a0, 4
> +#endif
> +
> /* Clear BSS */
> PTR_LA  a0, _edata
> PTR_LA  a2, _end

Regards
Jonas

Re: [PATCH] MIPS: Allow compressed images to be loaded at the usual address

[Jonas Gorski]

Hi,

On 5 February 2017 at 21:21, Alban  wrote:
> From: Alban Bedel 
>
> Normally compressed images have to be loaded at a different address to
> allow the decompressor to run. This add an option to let vmlinuz copy
> itself to the correct address from the normal vmlinux address.
>
> Signed-off-by: Alban Bedel 
> ---
>  arch/mips/Kconfig|  8 
>  arch/mips/boot/compressed/head.S | 13 +
>  2 files changed, 21 insertions(+)
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index b3c5bde..8074fc5 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -2961,6 +2961,14 @@ choice
> bool "Extend builtin kernel arguments with bootloader 
> arguments"
>  endchoice
>
> +config ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS
> +   bool "Load compressed images at the same address as uncompressed"
> +   depends on SYS_SUPPORTS_ZBOOT
> +   help
> + vmlinux and vmlinuz normally have different load addresses, with
> + this option vmlinuz expect to be loaded at the same address as
> + vmlinux.
> +
>  endmenu

Okay, it took me a while to understand the intention of this change. I
thought it was for supporting the case that VMLINUZ_LOAD_ADDRESS ==
VMLINUX_LOAD_ADDRESS, but it is indented for  VMLINUZ_LOAD_ADDRESS !=
VMLINUX_LOAD_ADDRESS, but still being loaded at VMLINUX_LOAD_ADDRESS.

So I guess that this can only happen with vmlinuz.bin, as vmlinux's
ELF header will cause it to be loaded at the expected address (for
sane bootloaders at least).

>  config LOCKDEP_SUPPORT
> diff --git a/arch/mips/boot/compressed/head.S 
> b/arch/mips/boot/compressed/head.S
> index 409cb48..a215171 100644
> --- a/arch/mips/boot/compressed/head.S
> +++ b/arch/mips/boot/compressed/head.S
> @@ -25,6 +25,19 @@ start:
> moves2, a2
> moves3, a3
>
> +#ifdef CONFIG_ZBOOT_VMLINUZ_AT_VMLINUX_LOAD_ADDRESS

With a bit of BAL trickery you could easily detect this at runtime and
then conditionally copy without requiring any additional config
symbols. Then you aren't limited to being executed from
VMLINUX_LOAD_ADDRESS.

> +   /* Move the text, data section and DTB to the correct address */
> +   PTR_LA  a0, .text
> +   PTR_LI  a1, VMLINUX_LOAD_ADDRESS
> +   PTR_LA  a2, _edata
> +0:
> +   lw  a3, 0(a1)
> +   sw  a3, 0(a0)
> +   addiu   a1, a1, 4
> +   bne a2, a0, 0b
> +addiu  a0, a0, 4
> +#endif
> +
> /* Clear BSS */
> PTR_LA  a0, _edata
> PTR_LA  a2, _end

Regards
Jonas

[PATCH] uapi glibc compat: fix outer guard of net device flags enum

[Jonas Gorski]

Fix a wrong condition preventing the higher net device flags
IFF_LOWER_UP etc to be defined if net/if.h is included before
linux/if.h.

The comment makes it clear the intention was to allow partial
definition with either parts.

This fixes compilation of userspace programs trying to use
IFF_LOWER_UP, IFF_DORMANT or IFF_ECHO.

Fixes: 4a91cb61bb99 ("uapi glibc compat: fix compile errors when glibc net/if.h 
included before linux/if.h")
Signed-off-by: Jonas Gorski <jonas.gor...@gmail.com>
---
Patch applies cleanly to both linus' HEAD and net-next. I wasn't sure
which one's the right one.

 include/uapi/linux/if.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index e601c8c..1158a04 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -31,7 +31,7 @@
 #include 
 
 /* For glibc compatibility. An empty enum does not compile. */
-#if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && \
+#if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 || \
 __UAPI_DEF_IF_NET_DEVICE_FLAGS != 0
 /**
  * enum net_device_flags -  net_device flags
@@ -99,7 +99,7 @@ enum net_device_flags {
IFF_ECHO= 1<<18, /* volatile */
 #endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO */
 };
-#endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && 
__UAPI_DEF_IF_NET_DEVICE_FLAGS != 0 */
+#endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 || 
__UAPI_DEF_IF_NET_DEVICE_FLAGS != 0 */
 
 /* for compatibility with glibc net/if.h */
 #if __UAPI_DEF_IF_NET_DEVICE_FLAGS
-- 
2.1.4

[PATCH] uapi glibc compat: fix outer guard of net device flags enum

[Jonas Gorski]

Fix a wrong condition preventing the higher net device flags
IFF_LOWER_UP etc to be defined if net/if.h is included before
linux/if.h.

The comment makes it clear the intention was to allow partial
definition with either parts.

This fixes compilation of userspace programs trying to use
IFF_LOWER_UP, IFF_DORMANT or IFF_ECHO.

Fixes: 4a91cb61bb99 ("uapi glibc compat: fix compile errors when glibc net/if.h 
included before linux/if.h")
Signed-off-by: Jonas Gorski 
---
Patch applies cleanly to both linus' HEAD and net-next. I wasn't sure
which one's the right one.

 include/uapi/linux/if.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/if.h b/include/uapi/linux/if.h
index e601c8c..1158a04 100644
--- a/include/uapi/linux/if.h
+++ b/include/uapi/linux/if.h
@@ -31,7 +31,7 @@
 #include 
 
 /* For glibc compatibility. An empty enum does not compile. */
-#if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && \
+#if __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 || \
 __UAPI_DEF_IF_NET_DEVICE_FLAGS != 0
 /**
  * enum net_device_flags -  net_device flags
@@ -99,7 +99,7 @@ enum net_device_flags {
IFF_ECHO= 1<<18, /* volatile */
 #endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO */
 };
-#endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 && 
__UAPI_DEF_IF_NET_DEVICE_FLAGS != 0 */
+#endif /* __UAPI_DEF_IF_NET_DEVICE_FLAGS_LOWER_UP_DORMANT_ECHO != 0 || 
__UAPI_DEF_IF_NET_DEVICE_FLAGS != 0 */
 
 /* for compatibility with glibc net/if.h */
 #if __UAPI_DEF_IF_NET_DEVICE_FLAGS
-- 
2.1.4

Re: [PATCH 06/20] usb: host: ehci-sead3: Support probing using device tree

[Jonas Gorski]

Hi,

On 9 August 2016 at 14:35, Paul Burton  wrote:
> Introduce support for probing the SEAD3 EHCI driver using device tree.
>
> Signed-off-by: Paul Burton 
> ---
>
>  drivers/usb/host/ehci-sead3.c | 29 +++--
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-sead3.c b/drivers/usb/host/ehci-sead3.c
> index 3d86cc2..05db1ae 100644
> --- a/drivers/usb/host/ehci-sead3.c
> +++ b/drivers/usb/host/ehci-sead3.c
> @@ -20,6 +20,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>
>  static int ehci_sead3_setup(struct usb_hcd *hcd)
> @@ -96,15 +97,25 @@ static int ehci_hcd_sead3_drv_probe(struct 
> platform_device *pdev)
>  {
> struct usb_hcd *hcd;
> struct resource *res;
> -   int ret;
> +   int ret, irq;
>
> if (usb_disabled())
> return -ENODEV;
>
> -   if (pdev->resource[1].flags != IORESOURCE_IRQ) {
> -   pr_debug("resource[1] is not IORESOURCE_IRQ");
> -   return -ENOMEM;
> +   if (pdev->dev.of_node) {
> +   irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +   if (!irq) {
> +   dev_err(>dev, "failed to map IRQ\n");
> +   return -ENODEV;
> +   }
> +   } else {
> +   if (pdev->resource[1].flags != IORESOURCE_IRQ) {
> +   pr_debug("resource[1] is not IORESOURCE_IRQ");
> +   return -ENOMEM;
> +   }
> +   irq = pdev->resource[1].start;
> }
> +

Why not just use platform_get_irq(pdev, 0) instead of this whole
block? Then you wouldn't need to care anymore whether resource 1 is
the IRQ resource (and avoid a potential overrun when the device has
only one resource), or if it is probed through of.

> hcd = usb_create_hcd(_sead3_hc_driver, >dev, "SEAD-3");
> if (!hcd)
> return -ENOMEM;
> @@ -121,8 +132,7 @@ static int ehci_hcd_sead3_drv_probe(struct 
> platform_device *pdev)
> /* Root hub has integrated TT. */
> hcd->has_tt = 1;
>
> -   ret = usb_add_hcd(hcd, pdev->resource[1].start,
> - IRQF_SHARED);
> +   ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> if (ret == 0) {
> platform_set_drvdata(pdev, hcd);
> device_wakeup_enable(hcd->self.controller);
> @@ -172,12 +182,19 @@ static const struct dev_pm_ops sead3_ehci_pmops = {
>  #define SEAD3_EHCI_PMOPS NULL
>  #endif
>
> +static const struct of_device_id sead3_ehci_of_match[] = {
> +   { .compatible = "mti,sead3-ehci" },

I don't see this compatible documented anywhere, please add binding
documentation for it first (and CC devicetree@vger ).

> +   {},
> +};
> +MODULE_DEVICE_TABLE(of, sead3_ehci_of_match);
> +
>  static struct platform_driver ehci_hcd_sead3_driver = {
> .probe  = ehci_hcd_sead3_drv_probe,
> .remove = ehci_hcd_sead3_drv_remove,
> .shutdown   = usb_hcd_platform_shutdown,
> .driver = {
> .name   = "sead3-ehci",
> +   .of_match_table = sead3_ehci_of_match,
> .pm = SEAD3_EHCI_PMOPS,
> }
>  };

Regards
Jonas

Re: [PATCH 06/20] usb: host: ehci-sead3: Support probing using device tree

[Jonas Gorski]

Hi,

On 9 August 2016 at 14:35, Paul Burton  wrote:
> Introduce support for probing the SEAD3 EHCI driver using device tree.
>
> Signed-off-by: Paul Burton 
> ---
>
>  drivers/usb/host/ehci-sead3.c | 29 +++--
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/host/ehci-sead3.c b/drivers/usb/host/ehci-sead3.c
> index 3d86cc2..05db1ae 100644
> --- a/drivers/usb/host/ehci-sead3.c
> +++ b/drivers/usb/host/ehci-sead3.c
> @@ -20,6 +20,7 @@
>   */
>
>  #include 
> +#include 
>  #include 
>
>  static int ehci_sead3_setup(struct usb_hcd *hcd)
> @@ -96,15 +97,25 @@ static int ehci_hcd_sead3_drv_probe(struct 
> platform_device *pdev)
>  {
> struct usb_hcd *hcd;
> struct resource *res;
> -   int ret;
> +   int ret, irq;
>
> if (usb_disabled())
> return -ENODEV;
>
> -   if (pdev->resource[1].flags != IORESOURCE_IRQ) {
> -   pr_debug("resource[1] is not IORESOURCE_IRQ");
> -   return -ENOMEM;
> +   if (pdev->dev.of_node) {
> +   irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> +   if (!irq) {
> +   dev_err(>dev, "failed to map IRQ\n");
> +   return -ENODEV;
> +   }
> +   } else {
> +   if (pdev->resource[1].flags != IORESOURCE_IRQ) {
> +   pr_debug("resource[1] is not IORESOURCE_IRQ");
> +   return -ENOMEM;
> +   }
> +   irq = pdev->resource[1].start;
> }
> +

Why not just use platform_get_irq(pdev, 0) instead of this whole
block? Then you wouldn't need to care anymore whether resource 1 is
the IRQ resource (and avoid a potential overrun when the device has
only one resource), or if it is probed through of.

> hcd = usb_create_hcd(_sead3_hc_driver, >dev, "SEAD-3");
> if (!hcd)
> return -ENOMEM;
> @@ -121,8 +132,7 @@ static int ehci_hcd_sead3_drv_probe(struct 
> platform_device *pdev)
> /* Root hub has integrated TT. */
> hcd->has_tt = 1;
>
> -   ret = usb_add_hcd(hcd, pdev->resource[1].start,
> - IRQF_SHARED);
> +   ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> if (ret == 0) {
> platform_set_drvdata(pdev, hcd);
> device_wakeup_enable(hcd->self.controller);
> @@ -172,12 +182,19 @@ static const struct dev_pm_ops sead3_ehci_pmops = {
>  #define SEAD3_EHCI_PMOPS NULL
>  #endif
>
> +static const struct of_device_id sead3_ehci_of_match[] = {
> +   { .compatible = "mti,sead3-ehci" },

I don't see this compatible documented anywhere, please add binding
documentation for it first (and CC devicetree@vger ).

> +   {},
> +};
> +MODULE_DEVICE_TABLE(of, sead3_ehci_of_match);
> +
>  static struct platform_driver ehci_hcd_sead3_driver = {
> .probe  = ehci_hcd_sead3_drv_probe,
> .remove = ehci_hcd_sead3_drv_remove,
> .shutdown   = usb_hcd_platform_shutdown,
> .driver = {
> .name   = "sead3-ehci",
> +   .of_match_table = sead3_ehci_of_match,
> .pm = SEAD3_EHCI_PMOPS,
> }
>  };

Regards
Jonas

Re: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Jonas Gorski]

Hi,

On 3 August 2016 at 13:02, Álvaro Fernández Rojas  wrote:
> Subject: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

Minor nitpick: I think the bindings should come first, then the
consumer. We shouldn't add support for it without having okayed the
binding.

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  .../devicetree/bindings/gpio/brcm,bcm6345-gpio.txt | 46 
> ++
>  1 file changed, 46 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt 
> b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> new file mode 100644
> index 000..48e35ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> @@ -0,0 +1,46 @@
> +Bindings for the Broadcom's brcm,bcm6345-gpio memory-mapped GPIO controllers.
> +
> +These bindings should only be used for BCM6338 and BCM6345 SoCs, since newer
> +ones need a proper pinctrl driver.

There's nothing wrong with using it on newer SoCs to have gpio now and
then later replacing it in the dts file with a pinctrl node once the
bindings/drivers are available. We won't/can't drop support for the
gpio-driver, as 6338/6345 don't have pinctrl anyway. So older dts's
with gpio nodes will continue to work, and newer dts's will use the
pinctrl drivers.

> +BCM6338 have 8-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +BCM6345 have 16-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +
> +Required properties:
> +   - compatible: should be "brcm,bcm6345-gpio"
> +   - reg-names: must contain
> +   "dat" - data register
> +   "dirout" - direction (output) register
> +   - reg: address + size pairs describing the GPIO register sets;
> +   order must correspond with the order of entries in reg-names
> +   - #gpio-cells: must be set to 2. The first cell is the pin number and
> +   the second cell is used to specify the gpio polarity:
> +   0 = active high
> +   1 = active low
> +   - gpio-controller: Marks the device node as a gpio controller.
> +
> +Optional properties:
> +   - big-endian: Memory is big endian.

I think we want native-endian here.

> +
> +Examples:
> +   - BCM6338:
> +   gpio0: gpio-controller@fffe0407 {

Since both have only one bank, it might make sense to call it just
"gpio" instead of numbering it.

> +   compatible = "brcm,bcm6345-gpio";
> +   reg-names = "dirout", "dat";
> +   reg = <0xfffe0407 1>, <0xfffe040f 1>;
> +
> +   #gpio-cells = <2>;
> +   gpio-controller;
> +   };
> +
> +   - BCM6345:
> +   gpio0: gpio-controller@fffe0406 {
> +   compatible = "brcm,bcm6345-gpio";
> +   reg-names = "dirout", "dat";
> +   reg = <0xfffe0406 2>, <0xfffe040a 2>;
> +   big-endian;
> +
> +   #gpio-cells = <2>;
> +   gpio-controller;
> +   };

Apart from those (minor) nitpicks, it looks good to me.


Regards
Jonas

Re: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

[Jonas Gorski]

Hi,

On 3 August 2016 at 13:02, Álvaro Fernández Rojas  wrote:
> Subject: [PATCH 2/2] gpio: dt-bindings: add brcm,bcm6345-gpio bindings

Minor nitpick: I think the bindings should come first, then the
consumer. We shouldn't add support for it without having okayed the
binding.

> This patch adds the device tree bindings for the Broadcom's BCM6345
> memory-mapped GPIO controllers.
>
> The gpios will be supported by gpio-mmio code of the
> GPIO generic library.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  .../devicetree/bindings/gpio/brcm,bcm6345-gpio.txt | 46 
> ++
>  1 file changed, 46 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt 
> b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> new file mode 100644
> index 000..48e35ec
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/brcm,bcm6345-gpio.txt
> @@ -0,0 +1,46 @@
> +Bindings for the Broadcom's brcm,bcm6345-gpio memory-mapped GPIO controllers.
> +
> +These bindings should only be used for BCM6338 and BCM6345 SoCs, since newer
> +ones need a proper pinctrl driver.

There's nothing wrong with using it on newer SoCs to have gpio now and
then later replacing it in the dts file with a pinctrl node once the
bindings/drivers are available. We won't/can't drop support for the
gpio-driver, as 6338/6345 don't have pinctrl anyway. So older dts's
with gpio nodes will continue to work, and newer dts's will use the
pinctrl drivers.

> +BCM6338 have 8-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +BCM6345 have 16-bit data and dirout registers, where GPIO state can be read
> +and/or written, and the direction changed from input to output.
> +
> +Required properties:
> +   - compatible: should be "brcm,bcm6345-gpio"
> +   - reg-names: must contain
> +   "dat" - data register
> +   "dirout" - direction (output) register
> +   - reg: address + size pairs describing the GPIO register sets;
> +   order must correspond with the order of entries in reg-names
> +   - #gpio-cells: must be set to 2. The first cell is the pin number and
> +   the second cell is used to specify the gpio polarity:
> +   0 = active high
> +   1 = active low
> +   - gpio-controller: Marks the device node as a gpio controller.
> +
> +Optional properties:
> +   - big-endian: Memory is big endian.

I think we want native-endian here.

> +
> +Examples:
> +   - BCM6338:
> +   gpio0: gpio-controller@fffe0407 {

Since both have only one bank, it might make sense to call it just
"gpio" instead of numbering it.

> +   compatible = "brcm,bcm6345-gpio";
> +   reg-names = "dirout", "dat";
> +   reg = <0xfffe0407 1>, <0xfffe040f 1>;
> +
> +   #gpio-cells = <2>;
> +   gpio-controller;
> +   };
> +
> +   - BCM6345:
> +   gpio0: gpio-controller@fffe0406 {
> +   compatible = "brcm,bcm6345-gpio";
> +   reg-names = "dirout", "dat";
> +   reg = <0xfffe0406 2>, <0xfffe040a 2>;
> +   big-endian;
> +
> +   #gpio-cells = <2>;
> +   gpio-controller;
> +   };

Apart from those (minor) nitpicks, it looks good to me.


Regards
Jonas

Re: [PATCH 6/6] MIPS: BMIPS: Add device tree example for BCM6362

[Jonas Gorski]

Hi,

On 13 June 2016 at 09:38, Álvaro Fernández Rojas  wrote:
> This adds a device tree example for SFR NeufBox 6.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  arch/mips/bmips/Kconfig|   4 +
>  arch/mips/boot/dts/brcm/Makefile   |   2 +
>  arch/mips/boot/dts/brcm/bcm6362.dtsi   | 134 
> +
>  arch/mips/boot/dts/brcm/bcm96362nb6ser.dts |  22 +
>  4 files changed, 162 insertions(+)
>  create mode 100644 arch/mips/boot/dts/brcm/bcm6362.dtsi
>  create mode 100644 arch/mips/boot/dts/brcm/bcm96362nb6ser.dts
>
> diff --git a/arch/mips/bmips/Kconfig b/arch/mips/bmips/Kconfig
> index 5b0ad8c..43da496 100644
> --- a/arch/mips/bmips/Kconfig
> +++ b/arch/mips/bmips/Kconfig
> @@ -33,6 +33,10 @@ config DT_BCM96358NB4SER
> bool "BCM96358NB4SER"
> select BUILTIN_DTB
>
> +config DT_BCM96362NB6SER
> +   bool "BCM96362NB6SER"

DT_SFR_NEUFBOX6_SERCOMM
   bool "SFR Neufbox 6 (Sercomm)"

maybe?

> +   select BUILTIN_DTB
> +
>  config DT_BCM96368MVWG
> bool "BCM96368MVWG"
> select BUILTIN_DTB
> diff --git a/arch/mips/boot/dts/brcm/Makefile 
> b/arch/mips/boot/dts/brcm/Makefile
> index c553b95..161e54b 100644
> --- a/arch/mips/boot/dts/brcm/Makefile
> +++ b/arch/mips/boot/dts/brcm/Makefile
> @@ -3,6 +3,7 @@ dtb-$(CONFIG_DT_BCM93384WVG)+= bcm93384wvg.dtb
>  dtb-$(CONFIG_DT_BCM93384WVG_VIPER) += bcm93384wvg_viper.dtb
>  dtb-$(CONFIG_DT_BCM963268VR3032U)  += bcm963268vr3032u.dtb
>  dtb-$(CONFIG_DT_BCM96358NB4SER)+= bcm96358nb4ser.dtb
> +dtb-$(CONFIG_DT_BCM96362NB6SER)+= bcm96362nb6ser.dtb

Similar here.

>  dtb-$(CONFIG_DT_BCM96368MVWG)  += bcm96368mvwg.dtb
>  dtb-$(CONFIG_DT_BCM9EJTAGPRB)  += bcm9ejtagprb.dtb
>  dtb-$(CONFIG_DT_BCM97125CBMB)  += bcm97125cbmb.dtb
> @@ -20,6 +21,7 @@ dtb-$(CONFIG_DT_NONE) += \
> bcm93384wvg_viper.dtb   \
> bcm963268vr3032u.dtb\
> bcm96358nb4ser.dtb  \
> +   bcm96362nb6ser.dtb  \
> bcm96368mvwg.dtb\
> bcm9ejtagprb.dtb\
> bcm97125cbmb.dtb\

(snip)

> diff --git a/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts 
> b/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts
> new file mode 100644
> index 000..a470230
> --- /dev/null
> +++ b/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts

Similar here.

> @@ -0,0 +1,22 @@
> +/dts-v1/;
> +
> +/include/ "bcm6362.dtsi"
> +
> +/ {
> +   compatible = "sfr,nb6-ser", "brcm,bcm6362";

I don't see the sfr vendor prefix documented (at least not in
4.7-rc2). If it is already in -next, then disregard that comment.


> +   model = "SFT NeufBox 6 (Sercomm)";

SFT? Not SFR? ;p



Regards
Jonas

Re: [PATCH 6/6] MIPS: BMIPS: Add device tree example for BCM6362

[Jonas Gorski]

Hi,

On 13 June 2016 at 09:38, Álvaro Fernández Rojas  wrote:
> This adds a device tree example for SFR NeufBox 6.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  arch/mips/bmips/Kconfig|   4 +
>  arch/mips/boot/dts/brcm/Makefile   |   2 +
>  arch/mips/boot/dts/brcm/bcm6362.dtsi   | 134 
> +
>  arch/mips/boot/dts/brcm/bcm96362nb6ser.dts |  22 +
>  4 files changed, 162 insertions(+)
>  create mode 100644 arch/mips/boot/dts/brcm/bcm6362.dtsi
>  create mode 100644 arch/mips/boot/dts/brcm/bcm96362nb6ser.dts
>
> diff --git a/arch/mips/bmips/Kconfig b/arch/mips/bmips/Kconfig
> index 5b0ad8c..43da496 100644
> --- a/arch/mips/bmips/Kconfig
> +++ b/arch/mips/bmips/Kconfig
> @@ -33,6 +33,10 @@ config DT_BCM96358NB4SER
> bool "BCM96358NB4SER"
> select BUILTIN_DTB
>
> +config DT_BCM96362NB6SER
> +   bool "BCM96362NB6SER"

DT_SFR_NEUFBOX6_SERCOMM
   bool "SFR Neufbox 6 (Sercomm)"

maybe?

> +   select BUILTIN_DTB
> +
>  config DT_BCM96368MVWG
> bool "BCM96368MVWG"
> select BUILTIN_DTB
> diff --git a/arch/mips/boot/dts/brcm/Makefile 
> b/arch/mips/boot/dts/brcm/Makefile
> index c553b95..161e54b 100644
> --- a/arch/mips/boot/dts/brcm/Makefile
> +++ b/arch/mips/boot/dts/brcm/Makefile
> @@ -3,6 +3,7 @@ dtb-$(CONFIG_DT_BCM93384WVG)+= bcm93384wvg.dtb
>  dtb-$(CONFIG_DT_BCM93384WVG_VIPER) += bcm93384wvg_viper.dtb
>  dtb-$(CONFIG_DT_BCM963268VR3032U)  += bcm963268vr3032u.dtb
>  dtb-$(CONFIG_DT_BCM96358NB4SER)+= bcm96358nb4ser.dtb
> +dtb-$(CONFIG_DT_BCM96362NB6SER)+= bcm96362nb6ser.dtb

Similar here.

>  dtb-$(CONFIG_DT_BCM96368MVWG)  += bcm96368mvwg.dtb
>  dtb-$(CONFIG_DT_BCM9EJTAGPRB)  += bcm9ejtagprb.dtb
>  dtb-$(CONFIG_DT_BCM97125CBMB)  += bcm97125cbmb.dtb
> @@ -20,6 +21,7 @@ dtb-$(CONFIG_DT_NONE) += \
> bcm93384wvg_viper.dtb   \
> bcm963268vr3032u.dtb\
> bcm96358nb4ser.dtb  \
> +   bcm96362nb6ser.dtb  \
> bcm96368mvwg.dtb\
> bcm9ejtagprb.dtb\
> bcm97125cbmb.dtb\

(snip)

> diff --git a/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts 
> b/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts
> new file mode 100644
> index 000..a470230
> --- /dev/null
> +++ b/arch/mips/boot/dts/brcm/bcm96362nb6ser.dts

Similar here.

> @@ -0,0 +1,22 @@
> +/dts-v1/;
> +
> +/include/ "bcm6362.dtsi"
> +
> +/ {
> +   compatible = "sfr,nb6-ser", "brcm,bcm6362";

I don't see the sfr vendor prefix documented (at least not in
4.7-rc2). If it is already in -next, then disregard that comment.


> +   model = "SFT NeufBox 6 (Sercomm)";

SFT? Not SFR? ;p



Regards
Jonas

Re: [PATCH 1/6] MIPS: BMIPS: add missing console to bcm96358nb4ser

[Jonas Gorski]

Hi Álvaro,

On 13 June 2016 at 09:38, Álvaro Fernández Rojas  wrote:
> Console definition is needed in order to avoid a warning in earlycon to
> console transition.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  arch/mips/boot/dts/brcm/bcm96358nb4ser.dts | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts 
> b/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts
> index f412117..702eae2 100644
> --- a/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts
> +++ b/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts

Unfortunately I haven't seen this earlier, but please don't name the
dts file bcm9* unless it's an actual reference/eval board from
Broadcom.

A more sensible name would be neufbox4-sercom.dts or
bcm6358-neufbox4-sercom.dts.


Regards
Jonas

Re: [PATCH 1/6] MIPS: BMIPS: add missing console to bcm96358nb4ser

[Jonas Gorski]

Hi Álvaro,

On 13 June 2016 at 09:38, Álvaro Fernández Rojas  wrote:
> Console definition is needed in order to avoid a warning in earlycon to
> console transition.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  arch/mips/boot/dts/brcm/bcm96358nb4ser.dts | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts 
> b/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts
> index f412117..702eae2 100644
> --- a/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts
> +++ b/arch/mips/boot/dts/brcm/bcm96358nb4ser.dts

Unfortunately I haven't seen this earlier, but please don't name the
dts file bcm9* unless it's an actual reference/eval board from
Broadcom.

A more sensible name would be neufbox4-sercom.dts or
bcm6358-neufbox4-sercom.dts.


Regards
Jonas

Re: [PATCH 2/3] MIPS: BMIPS: Add BCM6345 support

[Jonas Gorski]

Hi,

On 3 June 2016 at 10:12, Álvaro Fernández Rojas  wrote:
> BCM6345 has only one CPU, so SMP support must be disabled.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  Documentation/devicetree/bindings/mips/brcm/soc.txt | 2 +-
>  arch/mips/bmips/setup.c | 9 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.txt 
> b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> index 4a7e030..1936e8a 100644
> --- a/Documentation/devicetree/bindings/mips/brcm/soc.txt
> +++ b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> @@ -4,7 +4,7 @@ Required properties:
>
>  - compatible: "brcm,bcm3384", "brcm,bcm33843"
>"brcm,bcm3384-viper", "brcm,bcm33843-viper"
> -  "brcm,bcm6328", "brcm,bcm6358", "brcm,bcm6368",
> +  "brcm,bcm6328", "brcm,bcm6345", "brcm,bcm6358", "brcm,bcm6368",
>"brcm,bcm63168", "brcm,bcm63268",
>"brcm,bcm7125", "brcm,bcm7346", "brcm,bcm7358", "brcm,bcm7360",
>"brcm,bcm7362", "brcm,bcm7420", "brcm,bcm7425"

This is reasonable.

> diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
> index f146d12..b0d339d 100644
> --- a/arch/mips/bmips/setup.c
> +++ b/arch/mips/bmips/setup.c
> @@ -95,6 +95,14 @@ static void bcm6328_quirks(void)
> bcm63xx_fixup_cpu1();
>  }
>
> +static void bcm6345_quirks(void)
> +{
> +   /*
> +* BCM6345 has only one CPU and no SMP support
> +*/
> +   bmips_smp_enabled = 0;
> +}
> +
>  static void bcm6358_quirks(void)
>  {
> /*
> @@ -113,6 +121,7 @@ static const struct bmips_quirk bmips_quirk_list[] = {
> { "brcm,bcm3384-viper", _viper_quirks   },
> { "brcm,bcm33843-viper",_viper_quirks   },
> { "brcm,bcm6328",   _quirks },
> +   { "brcm,bcm6345",   _quirks },
> { "brcm,bcm6358",   _quirks },
> { "brcm,bcm6368",   _quirks },
> { "brcm,bcm63168",  _quirks },

This part is unnecessary, as cpu-bmips will only try to enable smp for
bmips4350 and higher. but not for bmips32/bmips3300.


Jonas

Re: [PATCH 2/3] MIPS: BMIPS: Add BCM6345 support

[Jonas Gorski]

Hi,

On 3 June 2016 at 10:12, Álvaro Fernández Rojas  wrote:
> BCM6345 has only one CPU, so SMP support must be disabled.
>
> Signed-off-by: Álvaro Fernández Rojas 
> ---
>  Documentation/devicetree/bindings/mips/brcm/soc.txt | 2 +-
>  arch/mips/bmips/setup.c | 9 +
>  2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/mips/brcm/soc.txt 
> b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> index 4a7e030..1936e8a 100644
> --- a/Documentation/devicetree/bindings/mips/brcm/soc.txt
> +++ b/Documentation/devicetree/bindings/mips/brcm/soc.txt
> @@ -4,7 +4,7 @@ Required properties:
>
>  - compatible: "brcm,bcm3384", "brcm,bcm33843"
>"brcm,bcm3384-viper", "brcm,bcm33843-viper"
> -  "brcm,bcm6328", "brcm,bcm6358", "brcm,bcm6368",
> +  "brcm,bcm6328", "brcm,bcm6345", "brcm,bcm6358", "brcm,bcm6368",
>"brcm,bcm63168", "brcm,bcm63268",
>"brcm,bcm7125", "brcm,bcm7346", "brcm,bcm7358", "brcm,bcm7360",
>"brcm,bcm7362", "brcm,bcm7420", "brcm,bcm7425"

This is reasonable.

> diff --git a/arch/mips/bmips/setup.c b/arch/mips/bmips/setup.c
> index f146d12..b0d339d 100644
> --- a/arch/mips/bmips/setup.c
> +++ b/arch/mips/bmips/setup.c
> @@ -95,6 +95,14 @@ static void bcm6328_quirks(void)
> bcm63xx_fixup_cpu1();
>  }
>
> +static void bcm6345_quirks(void)
> +{
> +   /*
> +* BCM6345 has only one CPU and no SMP support
> +*/
> +   bmips_smp_enabled = 0;
> +}
> +
>  static void bcm6358_quirks(void)
>  {
> /*
> @@ -113,6 +121,7 @@ static const struct bmips_quirk bmips_quirk_list[] = {
> { "brcm,bcm3384-viper", _viper_quirks   },
> { "brcm,bcm33843-viper",_viper_quirks   },
> { "brcm,bcm6328",   _quirks },
> +   { "brcm,bcm6345",   _quirks },
> { "brcm,bcm6358",   _quirks },
> { "brcm,bcm6368",   _quirks },
> { "brcm,bcm63168",  _quirks },

This part is unnecessary, as cpu-bmips will only try to enable smp for
bmips4350 and higher. but not for bmips32/bmips3300.


Jonas

Re: [PATCH RFC 2/2] MIPS: dt: Explicitly specify native endian behaviour for syscon

[Jonas Gorski]

Hi,

On 27 January 2016 at 00:16, Florian Fainelli  wrote:
> On 26/01/16 14:46, Mark Brown wrote:
>> On many MIPS systems the endianness of IP blocks is kept the same as
>> that of the CPU by the hardware.  This includes the system controllers
>> on these systems which are controlled via syscon which uses the regmap
>> API which used readl() and writel() to interact with the hardware,
>> meaning that all writes are converted to little endian when writing to
>> the hardware.  This caused a bad interaction with the regmap core in big
>> endian mode since it was not aware of the byte swapping and so ended up
>> performing little endian writes.
>>
>> Unfortunately when this issue was noticed it was addressed by updating
>> the DT for the affected devices to specify them as little endian.  This
>> happened to work since it resulted in two endianness swaps which
>> cancelled each other out and gave little endian behaviour but meant that
>> the DT was clearly not accurately describing the hardware.
>>
>> The intention of commit 29bb45f25ff305 (regmap-mmio: Use native
>> endianness for read/write) was to fix this by making regmap default to
>> native endianness but this breaks most other MMIO users where the
>> hardware has a fixed endianness and the implementation uses the __raw
>> accessors which are not intended to be used outside of architecture
>> code.  Instead use the newly added native-endian DT property to say
>> exactly what we want for these systems.
>>
>> Fixes: 29bb45f25ff305 (regmap-mmio: Use native endianness for read/write)
>> Reported-by: Johannes Berg 
>> Signed-off-by: Mark Brown 
>> ---
>>
>> Posted for review only, this will interact with some other patches
>> fixing the implementation of regmap-mmio and will probably need to be
>> merged along with them.
>>
>>  arch/mips/boot/dts/brcm/bcm6328.dtsi | 1 +
>
> v4.5-rc1 now contains an arch/mips/boot/dts/brcm/bcm6368.dtsi which
> copied the 6328.dtsi and therefore needs this hunk to be added to your
> patch series:
>
> diff --git a/arch/mips/boot/dts/brcm/bcm6368.dtsi
> b/arch/mips/boot/dts/brcm/bcm6368.dtsi
> index 9c8d3fe28b31..1f6b9b5cddb4 100644
> --- a/arch/mips/boot/dts/brcm/bcm6368.dtsi
> +++ b/arch/mips/boot/dts/brcm/bcm6368.dtsi
> @@ -54,7 +54,7 @@
> periph_cntl: syscon@1000 {
> compatible = "syscon";
> reg = <0x1000 0x14>;
> -   little-endian;
> +   native-endian;

But native-endian == big-endian usually for bcm63xx, so is this
actually a bugfix?


Jonas

Re: [PATCH RFC 2/2] MIPS: dt: Explicitly specify native endian behaviour for syscon

[Jonas Gorski]

Hi,

On 27 January 2016 at 00:16, Florian Fainelli  wrote:
> On 26/01/16 14:46, Mark Brown wrote:
>> On many MIPS systems the endianness of IP blocks is kept the same as
>> that of the CPU by the hardware.  This includes the system controllers
>> on these systems which are controlled via syscon which uses the regmap
>> API which used readl() and writel() to interact with the hardware,
>> meaning that all writes are converted to little endian when writing to
>> the hardware.  This caused a bad interaction with the regmap core in big
>> endian mode since it was not aware of the byte swapping and so ended up
>> performing little endian writes.
>>
>> Unfortunately when this issue was noticed it was addressed by updating
>> the DT for the affected devices to specify them as little endian.  This
>> happened to work since it resulted in two endianness swaps which
>> cancelled each other out and gave little endian behaviour but meant that
>> the DT was clearly not accurately describing the hardware.
>>
>> The intention of commit 29bb45f25ff305 (regmap-mmio: Use native
>> endianness for read/write) was to fix this by making regmap default to
>> native endianness but this breaks most other MMIO users where the
>> hardware has a fixed endianness and the implementation uses the __raw
>> accessors which are not intended to be used outside of architecture
>> code.  Instead use the newly added native-endian DT property to say
>> exactly what we want for these systems.
>>
>> Fixes: 29bb45f25ff305 (regmap-mmio: Use native endianness for read/write)
>> Reported-by: Johannes Berg 
>> Signed-off-by: Mark Brown 
>> ---
>>
>> Posted for review only, this will interact with some other patches
>> fixing the implementation of regmap-mmio and will probably need to be
>> merged along with them.
>>
>>  arch/mips/boot/dts/brcm/bcm6328.dtsi | 1 +
>
> v4.5-rc1 now contains an arch/mips/boot/dts/brcm/bcm6368.dtsi which
> copied the 6328.dtsi and therefore needs this hunk to be added to your
> patch series:
>
> diff --git a/arch/mips/boot/dts/brcm/bcm6368.dtsi
> b/arch/mips/boot/dts/brcm/bcm6368.dtsi
> index 9c8d3fe28b31..1f6b9b5cddb4 100644
> --- a/arch/mips/boot/dts/brcm/bcm6368.dtsi
> +++ b/arch/mips/boot/dts/brcm/bcm6368.dtsi
> @@ -54,7 +54,7 @@
> periph_cntl: syscon@1000 {
> compatible = "syscon";
> reg = <0x1000 0x14>;
> -   little-endian;
> +   native-endian;

But native-endian == big-endian usually for bcm63xx, so is this
actually a bugfix?


Jonas

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

On Fri, Dec 11, 2015 at 11:24 PM, Simon Arlott  wrote:
> On 11/12/15 22:02, Jonas Gorski wrote:
>> Hi,
>>
>> On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott  wrote:
>>> Broadcom BCM963xx boards have multiple nvram variants across different
>>> SoCs with additional checksum fields added whenever the size of the
>>> nvram was extended.
>>>
>>> Add this structure as a header file so that multiple drivers and userspace
>>> can use it.
>>>
>>> Signed-off-by: Simon Arlott 
>>> ---
>>> v3: Fix includes/type names, add comments explaining the nvram struct.
>>>
>>> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>>>
>>>  MAINTAINERS |  1 +
>>>  include/uapi/linux/bcm963xx_nvram.h | 53 
>>> +
>>>  2 files changed, 54 insertions(+)
>>>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 6b6d4e2e..abf18b4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2393,6 +2393,7 @@ F:drivers/irqchip/irq-bcm63*
>>>  F: drivers/irqchip/irq-bcm7*
>>>  F: drivers/irqchip/irq-brcmstb*
>>>  F: include/linux/bcm63xx_wdt.h
>>> +F: include/uapi/linux/bcm963xx_nvram.h
>>>
>>>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>>>  M: Prashant Sreedharan 
>>> diff --git a/include/uapi/linux/bcm963xx_nvram.h 
>>> b/include/uapi/linux/bcm963xx_nvram.h
>>> new file mode 100644
>>> index 000..2dcb307
>>> --- /dev/null
>>> +++ b/include/uapi/linux/bcm963xx_nvram.h
>>
>> Why uapi? The nvram layout isn't really enforced to be that way, and
>> at least Huawei uses a modified one on some devices (in case you
>> wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
>> should be kept for in-kernel use only.
>
> Because Florian suggested include/uapi/linux/bcm963xx_nvram.h; I could
> move it to include/linux/ instead if this is preferred.
>
>>> @@ -0,0 +1,53 @@
>>> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +/*
>>> + * Broadcom BCM963xx SoC board nvram data structure.
>>> + *
>>> + * The nvram structure varies in size depending on the SoC board version. 
>>> Use
>>> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
>>> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
>>> + *
>>> + * The "version" field value maps directly to the size and checksum names, 
>>> e.g.
>>> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE 
>>> bytes.
>>> + *
>>> + * Do not use the __reserved fields, especially not as an offset for CRC
>>> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed 
>>> or
>>> + * repositioned.

Because I just saw that: Nobody will read that. ;p

>>> + */
>>> +
>>> +#define BCM963XX_NVRAM_V4_SIZE 300
>>> +#define BCM963XX_NVRAM_V5_SIZE 1024
>>> +#define BCM963XX_NVRAM_V6_SIZE BCM963XX_NVRAM_V5_SIZE
>>> +#define BCM963XX_NVRAM_V7_SIZE 3072
>>> +
>>> +#define BCM963XX_NVRAM_NR_PARTS5
>>> +
>>> +struct bcm963xx_nvram {
>>> +   __u32   version;
>>> +   charbootline[256];
>>> +   charname[16];
>>> +   __u32   main_tp_number;
>>> +   __u32   psi_size;
>>> +   __u32   mac_addr_count;
>>> +   __u8mac_addr_base[ETH_ALEN];
>>> +   __u8__reserved1[2];
>>> +   __u32   checksum_v4;
>>> +
>>> +   __u8__reserved2[292];
>>> +   __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
>>> +   __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
>>> +   __u8__reserved3[388];
>>> +   union {
>>> +   __u32   checksum_v5;
>>> +   __u32   checksum_v6;
>>> +   };
>>
>> what's the point of this union? Both are the same size and have the
>> same function.
>
> For convenience when deciding which size of nvram to use.
>
> The mach-bcm63xx code uses the V5 definitions because it supports
> checksums at the v4 and v5 sizes.
>
> The bcm963xxpart code uses 

Re: [PATCH linux-next (v3) 3/3] mtd: part: Add BCM962368 CFE partitioning support

[Jonas Gorski]

for the second rootfs to allow it to be
> + updated.
> +   "rootfs_other":   The other (not currently selected) rootfs
> +
> + A decision is made regarding which of the two rootfs is to be
> + used based on the nvram data.
> +
>  config MTD_BCM47XX_PARTS
> tristate "BCM47XX partitioning support"
> depends on BCM47XX || ARCH_BCM_5301X
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f0f4140 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_MTD_CMDLINE_PARTS) += cmdlinepart.o
>  obj-$(CONFIG_MTD_AFS_PARTS)+= afs.o
>  obj-$(CONFIG_MTD_AR7_PARTS)+= ar7part.o
>  obj-$(CONFIG_MTD_BCM63XX_PARTS)+= bcm63xxpart.o
> +obj-$(CONFIG_MTD_BCM963268_PARTS) += bcm963268part.o
>  obj-$(CONFIG_MTD_BCM47XX_PARTS)+= bcm47xxpart.o
>
>  # 'Users' - code which presents functionality to userspace.
> diff --git a/drivers/mtd/bcm963268part.c b/drivers/mtd/bcm963268part.c
> new file mode 100644
> index 000..2740a48
> --- /dev/null
> +++ b/drivers/mtd/bcm963268part.c
> @@ -0,0 +1,350 @@
> +/*
> + * BCM963268 CFE image tag parser
> + * Copyright 2015 Simon Arlott
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * Derived from drivers/mtd/bcm63xxpart.c:
> + * Copyright © 2006-2008  Florian Fainelli 
> + *   Mike Albon 
> + * Copyright © 2009-2010  Daniel Dickinson 
> + * Copyright © 2011-2013  Jonas Gorski 
> + *
> + * Derived from 
> bcm963xx_4.12L.06B_consumer/bcmdrivers/opensource/char/board/bcm963xx/impl1/board.c,
> + * 
> bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/bcm_hwdefs.h:
> + * Copyright (c) 2002 Broadcom Corporation
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 

Why not move that one as well? Then you don't need to depend on MIPS
for COMPILE_TEST.

> +#include 
> +
> +/* Extended flash address, needs to be subtracted from bcm_tag flash offsets 
> */
> +#define BCM63XX_EXTENDED_SIZE  0xBFC0

This should be in bcm963xx_tag.h.
> +
> +#define BCM63XX_CFE_MAGIC_OFFSET   0x4e0
> +#define BCM963XX_CFE_VERSION_OFFSET0x570
> +#define BCM963XX_NVRAM_OFFSET  0x580

You should decide whether you want to call it BCM63XX (the SoC) or
BCM963XX (the Board using the SoC). Also probably better served in
bcm963xx_nvram.h


> +
> +enum bcm963268_nvram_part {
> +   PART_BOOT = 0,
> +   PART_ROOTFS_1,
> +   PART_ROOTFS_2,
> +   PART_DATA,
> +   PART_BBT,
> +};

This should be in bcm963xx_nvram.h.

> +
> +/* Ensure strings read from flash structs are null terminated */
> +#define STR_NULL_TERMINATE(x) \
> +   do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0)
> +
> +static int bcm963268_detect_cfe(struct mtd_info *master)
> +{
> +   char buf[9];
> +   int ret;
> +   size_t retlen;
> +
> +   ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, ,
> +  (void *)buf);
> +   buf[retlen] = 0;
> +
> +   if (ret < 0)
> +   return ret;
> +
> +   if (strncmp("cfe-v", buf, 5) == 0)
> +   return 0;
> +
> +   return -EINVAL;
> +}
> +
> +static int bcm963268_read_nvram(struct mtd_info *master,
> +   struct bcm963xx_nvram *nvram)
> +{
> +   u32 crc, expected_crc;
> +   size_t retlen;
> +   int ret;
> +
> +   /* extract nvram data */
> +   ret = mtd_read(master, BCM963XX_NVRAM_OFFSET, BCM963XX_NVRAM_V6_SIZE,
> +   , (void *)nvram);
> +
> +   if (ret < 0)
> +   return ret;
> +
> +   if (nvram->version < 6) {
> +   pr_warn("nvram version %d not supported\n", nvram->version);
> +   return -EINVAL;
> +   }
> +
> +   /* check checksum before using data */
> +   expected_crc = nvram->checksum_v6;
> +   nvram->checksum_v6 = 0;

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

Hi,

On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott  wrote:
> Broadcom BCM963xx boards have multiple nvram variants across different
> SoCs with additional checksum fields added whenever the size of the
> nvram was extended.
>
> Add this structure as a header file so that multiple drivers and userspace
> can use it.
>
> Signed-off-by: Simon Arlott 
> ---
> v3: Fix includes/type names, add comments explaining the nvram struct.
>
> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>
>  MAINTAINERS |  1 +
>  include/uapi/linux/bcm963xx_nvram.h | 53 
> +
>  2 files changed, 54 insertions(+)
>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b6d4e2e..abf18b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2393,6 +2393,7 @@ F:drivers/irqchip/irq-bcm63*
>  F: drivers/irqchip/irq-bcm7*
>  F: drivers/irqchip/irq-brcmstb*
>  F: include/linux/bcm63xx_wdt.h
> +F: include/uapi/linux/bcm963xx_nvram.h
>
>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>  M: Prashant Sreedharan 
> diff --git a/include/uapi/linux/bcm963xx_nvram.h 
> b/include/uapi/linux/bcm963xx_nvram.h
> new file mode 100644
> index 000..2dcb307
> --- /dev/null
> +++ b/include/uapi/linux/bcm963xx_nvram.h

Why uapi? The nvram layout isn't really enforced to be that way, and
at least Huawei uses a modified one on some devices (in case you
wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
should be kept for in-kernel use only.

> @@ -0,0 +1,53 @@
> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
> +
> +#include 
> +#include 
> +
> +/*
> + * Broadcom BCM963xx SoC board nvram data structure.
> + *
> + * The nvram structure varies in size depending on the SoC board version. Use
> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
> + *
> + * The "version" field value maps directly to the size and checksum names, 
> e.g.
> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
> + *
> + * Do not use the __reserved fields, especially not as an offset for CRC
> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
> + * repositioned.
> + */
> +
> +#define BCM963XX_NVRAM_V4_SIZE 300
> +#define BCM963XX_NVRAM_V5_SIZE 1024
> +#define BCM963XX_NVRAM_V6_SIZE BCM963XX_NVRAM_V5_SIZE
> +#define BCM963XX_NVRAM_V7_SIZE 3072
> +
> +#define BCM963XX_NVRAM_NR_PARTS5
> +
> +struct bcm963xx_nvram {
> +   __u32   version;
> +   charbootline[256];
> +   charname[16];
> +   __u32   main_tp_number;
> +   __u32   psi_size;
> +   __u32   mac_addr_count;
> +   __u8mac_addr_base[ETH_ALEN];
> +   __u8__reserved1[2];
> +   __u32   checksum_v4;
> +
> +   __u8__reserved2[292];
> +   __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
> +   __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
> +   __u8__reserved3[388];
> +   union {
> +   __u32   checksum_v5;
> +   __u32   checksum_v6;
> +   };

what's the point of this union? Both are the same size and have the
same function.

> +
> +   __u8__reserved4[2044];
> +   __u32   checksum_v7;
> +} __packed;

Why is it __packed? there are no unaligned members, so it should work
fine without this (and it did for bcm63xx).


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

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

Hi,

On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott  wrote:
> Broadcom BCM963xx boards have multiple nvram variants across different
> SoCs with additional checksum fields added whenever the size of the
> nvram was extended.
>
> Add this structure as a header file so that multiple drivers and userspace
> can use it.
>
> Signed-off-by: Simon Arlott 
> ---
> v3: Fix includes/type names, add comments explaining the nvram struct.
>
> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>
>  MAINTAINERS |  1 +
>  include/uapi/linux/bcm963xx_nvram.h | 53 
> +
>  2 files changed, 54 insertions(+)
>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6b6d4e2e..abf18b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2393,6 +2393,7 @@ F:drivers/irqchip/irq-bcm63*
>  F: drivers/irqchip/irq-bcm7*
>  F: drivers/irqchip/irq-brcmstb*
>  F: include/linux/bcm63xx_wdt.h
> +F: include/uapi/linux/bcm963xx_nvram.h
>
>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>  M: Prashant Sreedharan 
> diff --git a/include/uapi/linux/bcm963xx_nvram.h 
> b/include/uapi/linux/bcm963xx_nvram.h
> new file mode 100644
> index 000..2dcb307
> --- /dev/null
> +++ b/include/uapi/linux/bcm963xx_nvram.h

Why uapi? The nvram layout isn't really enforced to be that way, and
at least Huawei uses a modified one on some devices (in case you
wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
should be kept for in-kernel use only.

> @@ -0,0 +1,53 @@
> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
> +
> +#include 
> +#include 
> +
> +/*
> + * Broadcom BCM963xx SoC board nvram data structure.
> + *
> + * The nvram structure varies in size depending on the SoC board version. Use
> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
> + *
> + * The "version" field value maps directly to the size and checksum names, 
> e.g.
> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE bytes.
> + *
> + * Do not use the __reserved fields, especially not as an offset for CRC
> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed or
> + * repositioned.
> + */
> +
> +#define BCM963XX_NVRAM_V4_SIZE 300
> +#define BCM963XX_NVRAM_V5_SIZE 1024
> +#define BCM963XX_NVRAM_V6_SIZE BCM963XX_NVRAM_V5_SIZE
> +#define BCM963XX_NVRAM_V7_SIZE 3072
> +
> +#define BCM963XX_NVRAM_NR_PARTS5
> +
> +struct bcm963xx_nvram {
> +   __u32   version;
> +   charbootline[256];
> +   charname[16];
> +   __u32   main_tp_number;
> +   __u32   psi_size;
> +   __u32   mac_addr_count;
> +   __u8mac_addr_base[ETH_ALEN];
> +   __u8__reserved1[2];
> +   __u32   checksum_v4;
> +
> +   __u8__reserved2[292];
> +   __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
> +   __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
> +   __u8__reserved3[388];
> +   union {
> +   __u32   checksum_v5;
> +   __u32   checksum_v6;
> +   };

what's the point of this union? Both are the same size and have the
same function.

> +
> +   __u8__reserved4[2044];
> +   __u32   checksum_v7;
> +} __packed;

Why is it __packed? there are no unaligned members, so it should work
fine without this (and it did for bcm63xx).


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

Re: [PATCH linux-next (v3) 3/3] mtd: part: Add BCM962368 CFE partitioning support

[Jonas Gorski]

 flash area used
> + for the second rootfs to allow it to be
> + updated.
> +   "rootfs_other":   The other (not currently selected) rootfs
> +
> + A decision is made regarding which of the two rootfs is to be
> + used based on the nvram data.
> +
>  config MTD_BCM47XX_PARTS
> tristate "BCM47XX partitioning support"
> depends on BCM47XX || ARCH_BCM_5301X
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..f0f4140 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_MTD_CMDLINE_PARTS) += cmdlinepart.o
>  obj-$(CONFIG_MTD_AFS_PARTS)+= afs.o
>  obj-$(CONFIG_MTD_AR7_PARTS)+= ar7part.o
>  obj-$(CONFIG_MTD_BCM63XX_PARTS)+= bcm63xxpart.o
> +obj-$(CONFIG_MTD_BCM963268_PARTS) += bcm963268part.o
>  obj-$(CONFIG_MTD_BCM47XX_PARTS)+= bcm47xxpart.o
>
>  # 'Users' - code which presents functionality to userspace.
> diff --git a/drivers/mtd/bcm963268part.c b/drivers/mtd/bcm963268part.c
> new file mode 100644
> index 000..2740a48
> --- /dev/null
> +++ b/drivers/mtd/bcm963268part.c
> @@ -0,0 +1,350 @@
> +/*
> + * BCM963268 CFE image tag parser
> + * Copyright 2015 Simon Arlott
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + *
> + * Derived from drivers/mtd/bcm63xxpart.c:
> + * Copyright © 2006-2008  Florian Fainelli <flor...@openwrt.org>
> + *   Mike Albon <mal...@openwrt.org>
> + * Copyright © 2009-2010  Daniel Dickinson <open...@cshore.neomailbox.net>
> + * Copyright © 2011-2013  Jonas Gorski <jonas.gor...@gmail.com>
> + *
> + * Derived from 
> bcm963xx_4.12L.06B_consumer/bcmdrivers/opensource/char/board/bcm963xx/impl1/board.c,
> + * 
> bcm963xx_4.12L.06B_consumer/shared/opensource/include/bcm963xx/bcm_hwdefs.h:
> + * Copyright (c) 2002 Broadcom Corporation
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 

Why not move that one as well? Then you don't need to depend on MIPS
for COMPILE_TEST.

> +#include 
> +
> +/* Extended flash address, needs to be subtracted from bcm_tag flash offsets 
> */
> +#define BCM63XX_EXTENDED_SIZE  0xBFC0

This should be in bcm963xx_tag.h.
> +
> +#define BCM63XX_CFE_MAGIC_OFFSET   0x4e0
> +#define BCM963XX_CFE_VERSION_OFFSET0x570
> +#define BCM963XX_NVRAM_OFFSET  0x580

You should decide whether you want to call it BCM63XX (the SoC) or
BCM963XX (the Board using the SoC). Also probably better served in
bcm963xx_nvram.h


> +
> +enum bcm963268_nvram_part {
> +   PART_BOOT = 0,
> +   PART_ROOTFS_1,
> +   PART_ROOTFS_2,
> +   PART_DATA,
> +   PART_BBT,
> +};

This should be in bcm963xx_nvram.h.

> +
> +/* Ensure strings read from flash structs are null terminated */
> +#define STR_NULL_TERMINATE(x) \
> +   do { char *_str = (x); _str[sizeof(x) - 1] = 0; } while (0)
> +
> +static int bcm963268_detect_cfe(struct mtd_info *master)
> +{
> +   char buf[9];
> +   int ret;
> +   size_t retlen;
> +
> +   ret = mtd_read(master, BCM963XX_CFE_VERSION_OFFSET, 5, ,
> +  (void *)buf);
> +   buf[retlen] = 0;
> +
> +   if (ret < 0)
> +   return ret;
> +
> +   if (strncmp("cfe-v", buf, 5) == 0)
> +   return 0;
> +
> +   return -EINVAL;
> +}
> +
> +static int bcm963268_read_nvram(struct mtd_info *master,
> +   struct bcm963xx_nvram *nvram)
> +{
> +   u32 crc, expected_crc;
> +   size_t retlen;
> +   int ret;
> +
> +   /* extract nvram data */
> +   ret = mtd_read(master, BCM963XX_NVRAM_OFFSET, BCM963XX_NVRAM_V6_SIZE,
> +   , (void *)nvram);
> +
> +   if (ret < 0)
> +   return ret;
> +
> +   if (nvram->version < 6) {
> +   pr_warn("nvram version %d not supported\n", nvram->version);
> +   return -EINVAL;
> 

Re: [PATCH linux-next (v3) 1/3] MIPS: bcm963xx: Add Broadcom BCM963xx board nvram data structure

[Jonas Gorski]

On Fri, Dec 11, 2015 at 11:24 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
> On 11/12/15 22:02, Jonas Gorski wrote:
>> Hi,
>>
>> On Fri, Dec 11, 2015 at 10:54 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
>>> Broadcom BCM963xx boards have multiple nvram variants across different
>>> SoCs with additional checksum fields added whenever the size of the
>>> nvram was extended.
>>>
>>> Add this structure as a header file so that multiple drivers and userspace
>>> can use it.
>>>
>>> Signed-off-by: Simon Arlott <si...@fire.lp0.eu>
>>> ---
>>> v3: Fix includes/type names, add comments explaining the nvram struct.
>>>
>>> v2: Use external struct bcm963xx_nvram definition for bcm963268part.
>>>
>>>  MAINTAINERS |  1 +
>>>  include/uapi/linux/bcm963xx_nvram.h | 53 
>>> +
>>>  2 files changed, 54 insertions(+)
>>>  create mode 100644 include/uapi/linux/bcm963xx_nvram.h
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index 6b6d4e2e..abf18b4 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2393,6 +2393,7 @@ F:drivers/irqchip/irq-bcm63*
>>>  F: drivers/irqchip/irq-bcm7*
>>>  F: drivers/irqchip/irq-brcmstb*
>>>  F: include/linux/bcm63xx_wdt.h
>>> +F: include/uapi/linux/bcm963xx_nvram.h
>>>
>>>  BROADCOM TG3 GIGABIT ETHERNET DRIVER
>>>  M: Prashant Sreedharan <prash...@broadcom.com>
>>> diff --git a/include/uapi/linux/bcm963xx_nvram.h 
>>> b/include/uapi/linux/bcm963xx_nvram.h
>>> new file mode 100644
>>> index 000..2dcb307
>>> --- /dev/null
>>> +++ b/include/uapi/linux/bcm963xx_nvram.h
>>
>> Why uapi? The nvram layout isn't really enforced to be that way, and
>> at least Huawei uses a modified one on some devices (in case you
>> wondered why bcm63xx doesn't fail a crc32-"broken" one), so IMHO it
>> should be kept for in-kernel use only.
>
> Because Florian suggested include/uapi/linux/bcm963xx_nvram.h; I could
> move it to include/linux/ instead if this is preferred.
>
>>> @@ -0,0 +1,53 @@
>>> +#ifndef _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +#define _UAPI__LINUX_BCM963XX_NVRAM_H__
>>> +
>>> +#include 
>>> +#include 
>>> +
>>> +/*
>>> + * Broadcom BCM963xx SoC board nvram data structure.
>>> + *
>>> + * The nvram structure varies in size depending on the SoC board version. 
>>> Use
>>> + * the appropriate minimum BCM963XX_NVRAM_*_SIZE define for the information
>>> + * you need instead of sizeof(struct bcm963xx_nvram) as this may change.
>>> + *
>>> + * The "version" field value maps directly to the size and checksum names, 
>>> e.g.
>>> + * version 4 uses "checksum_v4" and the data is BCM963XX_NVRAM_V4_SIZE 
>>> bytes.
>>> + *
>>> + * Do not use the __reserved fields, especially not as an offset for CRC
>>> + * calculations (use BCM963XX_NVRAM_*_SIZE instead). These may be removed 
>>> or
>>> + * repositioned.

Because I just saw that: Nobody will read that. ;p

>>> + */
>>> +
>>> +#define BCM963XX_NVRAM_V4_SIZE 300
>>> +#define BCM963XX_NVRAM_V5_SIZE 1024
>>> +#define BCM963XX_NVRAM_V6_SIZE BCM963XX_NVRAM_V5_SIZE
>>> +#define BCM963XX_NVRAM_V7_SIZE 3072
>>> +
>>> +#define BCM963XX_NVRAM_NR_PARTS5
>>> +
>>> +struct bcm963xx_nvram {
>>> +   __u32   version;
>>> +   charbootline[256];
>>> +   charname[16];
>>> +   __u32   main_tp_number;
>>> +   __u32   psi_size;
>>> +   __u32   mac_addr_count;
>>> +   __u8mac_addr_base[ETH_ALEN];
>>> +   __u8__reserved1[2];
>>> +   __u32   checksum_v4;
>>> +
>>> +   __u8__reserved2[292];
>>> +   __u32   nand_part_offset[BCM963XX_NVRAM_NR_PARTS];
>>> +   __u32   nand_part_size[BCM963XX_NVRAM_NR_PARTS];
>>> +   __u8__reserved3[388];
>>> +   union {
>>> +   __u32   checksum_v5;
>>> +   __u32   checksum_v6;
>>> +   };
>>
>> what's the point of this union? Both are the same size and have the
>> same function.
>
> For convenience when deciding which size of nvram to use.
>
> The mach-bcm63xx code uses the V5 defi

Re: [RFC PATCH 3/7] doc: dt: mtd: partition: add on-flash format binding

[Jonas Gorski]

On Sat, Dec 5, 2015 at 6:19 AM, Brian Norris
 wrote:
> The platform description (such as the type of partition formats used on
> a given flash) should be done independently of the flash driver in use.
> However, we can't reasonably have *all* partition parsers run on all
> flash (until they find a match), so let's overload the 'partitions'
> subnode to support specifying which format(s) to try in the device tree.
>
> Start by supporting Google's (Chrome OS) FMAP structure.
>
> There have been others interested in extending devicetree support to
> other parsers, like the bcm47xxpart parser:
>
>   http://patchwork.ozlabs.org/patch/475986/
>
> and the AFS (ARM Flash Structure?) parser:
>
>   http://patchwork.ozlabs.org/patch/537827/
>
> Signed-off-by: Brian Norris 
> ---
>  .../devicetree/bindings/mtd/partition.txt  | 75 
> --
>  1 file changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt 
> b/Documentation/devicetree/bindings/mtd/partition.txt
> index 28ae56f5c972..1bf9a7243993 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -1,29 +1,56 @@
> -Representing flash partitions in devicetree
> +Flash partitions in device tree
> +===
>
> -Partitions can be represented by sub-nodes of an mtd device. This can be used
> +Flash devices can be partitioned into one or more functional ranges (e.g.,
> +"boot code", "nvram", and "kernel") in at least two distinct ways:
> +
> + (A) a fixed flash layout at production time or
> + (B) with an on-flash partition table, such as RedBoot, that describes the
> + geometry and naming/purpose of each functional region
> +
> +The former typically requires an operating system to learn about the
> +partitioning from some kind of metadata provided by the bootloader/firmware.
> +Such partitions can be described using the method in "Section A: Fixed 
> Partitions".
> +
> +The latter is somewhat analogous to partition tables used on block devices
> +(e.g., MBR or GPT), except that there is less standardization for flash
> +devices, and it is not always safe or efficient to attempt to search for all 
> of
> +them on every flash device in the system, particularly since many of them 
> allow
> +their data structures to be placed anywhere on the flash, and so require
> +scanning the entire flash device to find them.
> +
> +To assist system software in locating these partition tables, we provide a
> +binding to describe which partition format(s) may be used on a given flash,
> +found below in "Section B: On-Flash Partition Tables".
> +
> +
> +Section A: Fixed Partitions
> +---
> +
> +Partitions can be represented by sub-nodes of a flash device. This can be 
> used
>  on platforms which have strong conventions about which portions of a flash 
> are
>  used for what purposes, but which don't use an on-flash partition table such
>  as RedBoot.
>
> -The partition table should be a subnode of the mtd node and should be named
> +The partition table should be a subnode of the flash node and should be named
>  'partitions'. This node should have the following property:
>  - compatible : (required) must be "partitions"
>  Partitions are then defined in subnodes of the partitions node.
>
> -For backwards compatibility partitions as direct subnodes of the mtd device 
> are
> +For backwards compatibility partitions as direct subnodes of the flash 
> device are
>  supported. This use is discouraged.
>  NOTE: also for backwards compatibility, direct subnodes that have a 
> compatible
>  string are not considered partitions, as they may be used for other bindings.
>
>  #address-cells & #size-cells must both be present in the partitions subnode 
> of the
> -mtd device. There are two valid values for both:
> +flash device. There are two valid values for both:
>  <1>: for partitions that require a single 32-bit cell to represent their
>   size/address (aka the value is below 4 GiB)
>  <2>: for partitions that require two 32-bit cells to represent their
>   size/address (aka the value is 4 GiB or greater).
>
>  Required properties:
> -- reg : The partition's offset and size within the mtd bank.
> +- reg : The partition's offset and size within the flash
>
>  Optional properties:
>  - label : The label / name for this partition.  If omitted, the label is 
> taken
> @@ -89,3 +116,39 @@ flash@2 {
> };
> };
>  };
> +
> +
> +Section B: On-Flash Partition Tables
> +
> +
> +System designers use a variety of on-flash data structures to describe the
> +layout of the flash. Because it's not always optimal for system software to
> +scan for every sort of data structure that might be used, one can specify 
> which
> +structure(s) might be used on a given flash using the 'partitions' subnode of
> +the flash node.
> +
> +Node name: 

Re: [RFC PATCH 0/7] mtd: partitions: add of_match_table support

[Jonas Gorski]

Hi,

On Sat, Dec 5, 2015 at 6:19 AM, Brian Norris
 wrote:
> Hi,
>
> There have been several discussions [1] about adding a device tree binding for
> associating flash devices with the partition parser(s) that are used on the
> flash. There are a few reasons:
>
>  (1) drivers shouldn't have to be encoding platform knowledge by listing what
>  parsers might be used on a given system (this is the currently all that's
>  supported)
>  (2) we can't just scan for all supported parsers (like the block system 
> does), since
>  there is a wide diversity of "formats" (no standardization), and it is 
> not
>  always safe or efficient to attempt to do so, particularly since many of
>  them allow their data structures to be placed anywhere on the flash, and
>  so require scanning the entire flash device to find them.
>
> So instead, let's support a new binding so that a device tree can specify what
> partition formats might be used. This seems like a reasonable choice (even
> though it's not strictly a hardware description) because the flash layout /
> partitioning is often very closely tied with the bootloader/firmware, at
> production time.

On a first glance this looks good to me, and looks easily extensible
for application of non-complete partition parsers.

E.g. for the "brcm,bcm6345-imagetag" we would want to actually do something like

partitions {


partition@0 {
reg = <0x0 0x1>;
label = "cfe";
read-only;
};

partition@1 {
reg = <0x1 0x3d>;
label = "firmware";
compatible = "brcm,bcm6345-imagetag";
};

partition@3e {
reg = <0x3e 0x1>;
label = "art";
read-only;
};

   partition@3f {
reg = <0x3f 0x1>;
label = "nvram";
read-only;
};
};

as the image tag can only specify the offsets and sizes of the rootfs
and kernel parts, but not of any other parts.

>
> Also, as an example first-use of this mechanism, I support Google's FMAP flash
> structure, used on Chrome OS devices.
>
> Note that this is an RFC, mainly for the reason noted in patch 6 ("RFC: mtd:
> partitions: enable of_match_table matching"): the of_match_table support won't
> yet autoload a partition parser that is built as a module. I'm not quite sure
> if there's a lot of value in supporting MTD parsers as modules (block 
> partition
> support can't be), but that is supported for "by-name" parser lookups in MTD
> already, so I don't feel like dropping that feature yet. Tips or thoughts are
> particularly welcome on this aspect!

I would assume a lot of the cases these would be a chicken-egg
problem, you need the parser to be able to find and mount the rootfs,
but you you need mount the rootfs to load the parser.

> Also note that there's an existing undocumented binding for a
> "linux,part-probe" property, but it is only usable on the physmap_of.c driver
> at the moment, and it is IMO not a good binding. I posted my thoughts on that
> previously here [2], and since no one else cared to make a better one...I did
> it myself.
>
> I'd love it if we could kill the unreviewed binding off in favor of something
> more like this...

I agree fully that this is a bad binding, as it exposes internal names
that aren't supposed to be fixed.


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

Re: [RFC PATCH 0/7] mtd: partitions: add of_match_table support

[Jonas Gorski]

Hi,

On Sat, Dec 5, 2015 at 6:19 AM, Brian Norris
 wrote:
> Hi,
>
> There have been several discussions [1] about adding a device tree binding for
> associating flash devices with the partition parser(s) that are used on the
> flash. There are a few reasons:
>
>  (1) drivers shouldn't have to be encoding platform knowledge by listing what
>  parsers might be used on a given system (this is the currently all that's
>  supported)
>  (2) we can't just scan for all supported parsers (like the block system 
> does), since
>  there is a wide diversity of "formats" (no standardization), and it is 
> not
>  always safe or efficient to attempt to do so, particularly since many of
>  them allow their data structures to be placed anywhere on the flash, and
>  so require scanning the entire flash device to find them.
>
> So instead, let's support a new binding so that a device tree can specify what
> partition formats might be used. This seems like a reasonable choice (even
> though it's not strictly a hardware description) because the flash layout /
> partitioning is often very closely tied with the bootloader/firmware, at
> production time.

On a first glance this looks good to me, and looks easily extensible
for application of non-complete partition parsers.

E.g. for the "brcm,bcm6345-imagetag" we would want to actually do something like

partitions {


partition@0 {
reg = <0x0 0x1>;
label = "cfe";
read-only;
};

partition@1 {
reg = <0x1 0x3d>;
label = "firmware";
compatible = "brcm,bcm6345-imagetag";
};

partition@3e {
reg = <0x3e 0x1>;
label = "art";
read-only;
};

   partition@3f {
reg = <0x3f 0x1>;
label = "nvram";
read-only;
};
};

as the image tag can only specify the offsets and sizes of the rootfs
and kernel parts, but not of any other parts.

>
> Also, as an example first-use of this mechanism, I support Google's FMAP flash
> structure, used on Chrome OS devices.
>
> Note that this is an RFC, mainly for the reason noted in patch 6 ("RFC: mtd:
> partitions: enable of_match_table matching"): the of_match_table support won't
> yet autoload a partition parser that is built as a module. I'm not quite sure
> if there's a lot of value in supporting MTD parsers as modules (block 
> partition
> support can't be), but that is supported for "by-name" parser lookups in MTD
> already, so I don't feel like dropping that feature yet. Tips or thoughts are
> particularly welcome on this aspect!

I would assume a lot of the cases these would be a chicken-egg
problem, you need the parser to be able to find and mount the rootfs,
but you you need mount the rootfs to load the parser.

> Also note that there's an existing undocumented binding for a
> "linux,part-probe" property, but it is only usable on the physmap_of.c driver
> at the moment, and it is IMO not a good binding. I posted my thoughts on that
> previously here [2], and since no one else cared to make a better one...I did
> it myself.
>
> I'd love it if we could kill the unreviewed binding off in favor of something
> more like this...

I agree fully that this is a bad binding, as it exposes internal names
that aren't supposed to be fixed.


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

Re: [RFC PATCH 3/7] doc: dt: mtd: partition: add on-flash format binding

[Jonas Gorski]

On Sat, Dec 5, 2015 at 6:19 AM, Brian Norris
 wrote:
> The platform description (such as the type of partition formats used on
> a given flash) should be done independently of the flash driver in use.
> However, we can't reasonably have *all* partition parsers run on all
> flash (until they find a match), so let's overload the 'partitions'
> subnode to support specifying which format(s) to try in the device tree.
>
> Start by supporting Google's (Chrome OS) FMAP structure.
>
> There have been others interested in extending devicetree support to
> other parsers, like the bcm47xxpart parser:
>
>   http://patchwork.ozlabs.org/patch/475986/
>
> and the AFS (ARM Flash Structure?) parser:
>
>   http://patchwork.ozlabs.org/patch/537827/
>
> Signed-off-by: Brian Norris 
> ---
>  .../devicetree/bindings/mtd/partition.txt  | 75 
> --
>  1 file changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/partition.txt 
> b/Documentation/devicetree/bindings/mtd/partition.txt
> index 28ae56f5c972..1bf9a7243993 100644
> --- a/Documentation/devicetree/bindings/mtd/partition.txt
> +++ b/Documentation/devicetree/bindings/mtd/partition.txt
> @@ -1,29 +1,56 @@
> -Representing flash partitions in devicetree
> +Flash partitions in device tree
> +===
>
> -Partitions can be represented by sub-nodes of an mtd device. This can be used
> +Flash devices can be partitioned into one or more functional ranges (e.g.,
> +"boot code", "nvram", and "kernel") in at least two distinct ways:
> +
> + (A) a fixed flash layout at production time or
> + (B) with an on-flash partition table, such as RedBoot, that describes the
> + geometry and naming/purpose of each functional region
> +
> +The former typically requires an operating system to learn about the
> +partitioning from some kind of metadata provided by the bootloader/firmware.
> +Such partitions can be described using the method in "Section A: Fixed 
> Partitions".
> +
> +The latter is somewhat analogous to partition tables used on block devices
> +(e.g., MBR or GPT), except that there is less standardization for flash
> +devices, and it is not always safe or efficient to attempt to search for all 
> of
> +them on every flash device in the system, particularly since many of them 
> allow
> +their data structures to be placed anywhere on the flash, and so require
> +scanning the entire flash device to find them.
> +
> +To assist system software in locating these partition tables, we provide a
> +binding to describe which partition format(s) may be used on a given flash,
> +found below in "Section B: On-Flash Partition Tables".
> +
> +
> +Section A: Fixed Partitions
> +---
> +
> +Partitions can be represented by sub-nodes of a flash device. This can be 
> used
>  on platforms which have strong conventions about which portions of a flash 
> are
>  used for what purposes, but which don't use an on-flash partition table such
>  as RedBoot.
>
> -The partition table should be a subnode of the mtd node and should be named
> +The partition table should be a subnode of the flash node and should be named
>  'partitions'. This node should have the following property:
>  - compatible : (required) must be "partitions"
>  Partitions are then defined in subnodes of the partitions node.
>
> -For backwards compatibility partitions as direct subnodes of the mtd device 
> are
> +For backwards compatibility partitions as direct subnodes of the flash 
> device are
>  supported. This use is discouraged.
>  NOTE: also for backwards compatibility, direct subnodes that have a 
> compatible
>  string are not considered partitions, as they may be used for other bindings.
>
>  #address-cells & #size-cells must both be present in the partitions subnode 
> of the
> -mtd device. There are two valid values for both:
> +flash device. There are two valid values for both:
>  <1>: for partitions that require a single 32-bit cell to represent their
>   size/address (aka the value is below 4 GiB)
>  <2>: for partitions that require two 32-bit cells to represent their
>   size/address (aka the value is 4 GiB or greater).
>
>  Required properties:
> -- reg : The partition's offset and size within the mtd bank.
> +- reg : The partition's offset and size within the flash
>
>  Optional properties:
>  - label : The label / name for this partition.  If omitted, the label is 
> taken
> @@ -89,3 +116,39 @@ flash@2 {
> };
> };
>  };
> +
> +
> +Section B: On-Flash Partition Tables
> +
> +
> +System designers use a variety of on-flash data structures to describe the
> +layout of the flash. Because it's not always optimal for system software to
> +scan for every sort of data structure that might be used, one can specify 
> which
> +structure(s) might be used on a given flash using the 

Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

[Jonas Gorski]

On Thu, Dec 3, 2015 at 12:41 AM, Simon Arlott  wrote:
> Add device tree binding for NAND on the BCM6368.
>
> The BCM6368 has a NAND interrupt register with combined status and enable
> registers. It also requires a clock, so add an optional clock to the
> common brcmnand binding.
>
> Signed-off-by: Simon Arlott 
> ---
> Renamed from BCM63268, made clock a generic property.
>
>  .../devicetree/bindings/mtd/brcm,brcmnand.txt  | 32 
> ++
>  1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt 
> b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> index 4ff7128..16d7835 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> @@ -45,6 +45,8 @@ Required properties:
>  - #size-cells  : <0>
>
>  Optional properties:
> +- clock : reference to the clock for the NAND controller
> +- clock-names   : "nand" (required for the above clock)
>  - brcm,nand-has-wp  : Some versions of this IP include a 
> write-protect
>(WP) control bit. It is always available on >=
>v7.0. Use this property to describe the rare
> @@ -72,6 +74,12 @@ we define additional 'compatible' properties and 
> associated register resources w
> and enable registers
>   - reg-names: (required) "nand-int-base"
>
> +   * "brcm,nand-bcm6368"
> + - compatible: should contain "brcm,nand-bcm", "brcm,nand-bcm6368"
> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined 
> status
> +   and enable registers, and boot address registers
> + - reg-names: (required) "nand-intr-base"

Can't we use the same name as bcm63138, i.e. nand-int-base?

> +
> * "brcm,nand-iproc"
>   - reg: (required) the "IDM" register range, for interrupt enable and APB
> bus access endianness configuration, and the "EXT" register range,
> @@ -148,3 +156,27 @@ nand@f0442800 {
> };
> };
>  };
> +
> +nand@1200 {
> +   compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> +   "brcm,brcmnand-v4.0", "brcm,brcmnand";

I know it's now much too late, but this is IMHO a very odd way of
defining that this is a v4 nand, but uses bcm6368 compatible
interrupts, as bcm6368 is a much older, unsupported nand revision.


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

Re: [PATCH 1/3] mtd: brcmnand: Add brcm,bcm6368-nand device tree binding

[Jonas Gorski]

On Thu, Dec 3, 2015 at 12:41 AM, Simon Arlott  wrote:
> Add device tree binding for NAND on the BCM6368.
>
> The BCM6368 has a NAND interrupt register with combined status and enable
> registers. It also requires a clock, so add an optional clock to the
> common brcmnand binding.
>
> Signed-off-by: Simon Arlott 
> ---
> Renamed from BCM63268, made clock a generic property.
>
>  .../devicetree/bindings/mtd/brcm,brcmnand.txt  | 32 
> ++
>  1 file changed, 32 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt 
> b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> index 4ff7128..16d7835 100644
> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> @@ -45,6 +45,8 @@ Required properties:
>  - #size-cells  : <0>
>
>  Optional properties:
> +- clock : reference to the clock for the NAND controller
> +- clock-names   : "nand" (required for the above clock)
>  - brcm,nand-has-wp  : Some versions of this IP include a 
> write-protect
>(WP) control bit. It is always available on >=
>v7.0. Use this property to describe the rare
> @@ -72,6 +74,12 @@ we define additional 'compatible' properties and 
> associated register resources w
> and enable registers
>   - reg-names: (required) "nand-int-base"
>
> +   * "brcm,nand-bcm6368"
> + - compatible: should contain "brcm,nand-bcm", "brcm,nand-bcm6368"
> + - reg: (required) the 'NAND_INTR_BASE' register range, with combined 
> status
> +   and enable registers, and boot address registers
> + - reg-names: (required) "nand-intr-base"

Can't we use the same name as bcm63138, i.e. nand-int-base?

> +
> * "brcm,nand-iproc"
>   - reg: (required) the "IDM" register range, for interrupt enable and APB
> bus access endianness configuration, and the "EXT" register range,
> @@ -148,3 +156,27 @@ nand@f0442800 {
> };
> };
>  };
> +
> +nand@1200 {
> +   compatible = "brcm,nand-bcm63168", "brcm,nand-bcm6368",
> +   "brcm,brcmnand-v4.0", "brcm,brcmnand";

I know it's now much too late, but this is IMHO a very odd way of
defining that this is a v4 nand, but uses bcm6368 compatible
interrupts, as bcm6368 is a much older, unsupported nand revision.


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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

Hi,

On Wed, Dec 2, 2015 at 9:54 PM, Brian Norris
 wrote:
> Hi,
>
> On Wed, Dec 02, 2015 at 09:44:04PM +0100, Jonas Gorski wrote:
>> On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott  wrote:
>> > On 01/12/15 10:41, Jonas Gorski wrote:
>> >> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott  wrote:
>> >>> +
>> >>> +   /* Go to start of buffer */
>> >>> +   buf -= FC_WORDS;
>> >>> +
>> >>> +   /* Erased if all data bytes are 0xFF */
>> >>> +   buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL;
>> >>> +
>> >>> +   if (!buf_erased)
>> >>> +   goto out_free;
>> >>
>> >> We now have a function exactly for that use case in 4.4,
>> >> nand_check_erased_buf [1], consider using that. This also has the
>> >> benefit of treating bit flips as correctable as long as the ECC scheme
>> >> is strong enough.
>> >
>> > I have no idea whether or not it's appropriate to specify
>> > bitflips_threshold > 0 so it'd just be a more complex way to do
>> > a memchr_inv() search for 0xFF.
>>
>> The threshold would be the amount of bitflips the code can correct, so
>> basically ecc.strength (at least that is my understanding).
>>
>> > The code also has to check for the hamming code bytes being all 0x00,
>> > because according to the comments [2], the controller also has
>> > difficulty with the non-erased all-0xFFs scenario too.
>>
>> According to brcmnand.c hamming can fix up to fifteen bitflips, but in
>
> Hamming only protects 1 bitflip. The '15' is the value used by the
> controller to represent Hamming (i.e., there is no BCH-15).

Ah, yeah that confused me because I also vaguely remembered hamming
only providing protection for 1, but then saw the ecc_level = 15
assignment.

Still, that means that even hamming protected erased pages with a
single bitflip should be treated as readable / all-0xff, but with
correctable bitflips, and not as uncorrectable.

>> the current code you would fail a hamming protected all-0xff-page for
>> even a single bitflip in the data or in the ecc bytes, which means
>> that all-0xff-pages wouldn't be protected at all.
>
> BTW, I think Kamal had code to handle protecting bitflips in erased
> pages code in the Broadcom STB Linux BSP. Perhaps he can port that to
> upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably
> handle your case too, Simon, although it wouldn't be optimal for an
> all-0xff check (i.e., bitflip_threshold == 0).
>
> If that's really an issue (i.e., we have an implementation + data), I'm
> sure we could add optimization to nand_check_erased_ecc_chunk() to
> support the bitflip_threshold == 0 case.

Maybe I'm missing something, but wasn't the point of introducing
nand_check_erased_ecc_chunk that bitflips in erased pages should be
treated as bitflips corrected by the ecc, and therefore fixed up
before passing the data further on? So having a theshold of 0 would be
wrong / no protection at all, and could be quite destructive on MLC
nand, where bitflips in erased pages are rather common.


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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott  wrote:
> On 01/12/15 10:41, Jonas Gorski wrote:
>> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott  wrote:
>>> +
>>> +   /* Go to start of buffer */
>>> +   buf -= FC_WORDS;
>>> +
>>> +   /* Erased if all data bytes are 0xFF */
>>> +   buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL;
>>> +
>>> +   if (!buf_erased)
>>> +   goto out_free;
>>
>> We now have a function exactly for that use case in 4.4,
>> nand_check_erased_buf [1], consider using that. This also has the
>> benefit of treating bit flips as correctable as long as the ECC scheme
>> is strong enough.
>
> I have no idea whether or not it's appropriate to specify
> bitflips_threshold > 0 so it'd just be a more complex way to do
> a memchr_inv() search for 0xFF.

The threshold would be the amount of bitflips the code can correct, so
basically ecc.strength (at least that is my understanding).

> The code also has to check for the hamming code bytes being all 0x00,
> because according to the comments [2], the controller also has
> difficulty with the non-erased all-0xFFs scenario too.

According to brcmnand.c hamming can fix up to fifteen bitflips, but in
the current code you would fail a hamming protected all-0xff-page for
even a single bitflip in the data or in the ecc bytes, which means
that all-0xff-pages wouldn't be protected at all.


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

Re: [PATCH (v6) 1/2] mtd: brcmnand: Add brcm,bcm63268-nand device tree binding

[Jonas Gorski]

On Wed, Dec 2, 2015 at 8:05 PM, Brian Norris
 wrote:
> + Broadcom list + Kamal
>
> On Tue, Nov 24, 2015 at 08:19:37PM -, Simon Arlott wrote:
>> Add device tree binding for NAND on the BCM63268.
>>
>> The BCM63268 has a NAND interrupt register with combined status and enable
>> registers.
>>
>> Signed-off-by: Simon Arlott 
>> ---
>>  .../devicetree/bindings/mtd/brcm,brcmnand.txt  | 35 
>> ++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> index 4ff7128..f2a71c8 100644
>> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> @@ -72,6 +72,14 @@ we define additional 'compatible' properties and 
>> associated register resources w
>> and enable registers
>>   - reg-names: (required) "nand-int-base"
>>
>> +   * "brcm,nand-bcm63268"
>> + - compatible: should contain "brcm,nand-bcm", "brcm,nand-bcm63268"
>
> Looks like you're aiming to support bcm63168? Is bcm63268 the first
> chip to include this style of register then? The numbering seems
> backwards, but that may just be reality.

There are four chip variants, bcm63168, bcm63268, bcm63169 and
bcm63269, and they were all introduced at the same time. the *16x have
less features than *26x (IIRC only two rgmii ports instead of four, no
hw crypto, maybe no dect?), and the *9 ones have no xDSL support.

>From a registers location/layout, clocks, etc standpoint, there is no
difference between bcm63168 and bcm63268 (and the x9's).

As a reference, Broadcom uses bcm63268 as the "generic" name for the
chip family in their code, but on their website only advertise the
bcm63168. Both chips do exist though, and at least the bcm63169, I
have devices with these three here.


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

Re: [PATCH] Revert "of/irq: make of_irq_find_parent static"

[Jonas Gorski]

Hi,

On Wed, Dec 2, 2015 at 5:14 PM, Qais Yousef  wrote:
> This reverts commit 52493d446141b07c8ba28dd6a529513f8b2342bd.
>
> Signed-off-by: Qais Yousef 
>
> Conflicts:
> include/linux/of_irq.h
> ---
> I have a patch series that is under review that makes use of 
> of_irq_find_parent()
>
> The affected patch is this:
>
> https://lkml.org/lkml/2015/11/25/291
>
> Is it wrong to use this function? If yes, what's the alternative?
> If no, OK to revert?

Make the revert part of the series that needs it, so it won't get
moved again because of no external users.

also ...

>
> Thanks,
> Qais
>
>
>  drivers/of/irq.c   | 2 +-
>  include/linux/of_irq.h | 6 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 902b89be7217..45735d56e435 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -53,7 +53,7 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
>   * Returns a pointer to the interrupt parent node, or NULL if the interrupt
>   * parent could not be determined.
>   */
> -static struct device_node *of_irq_find_parent(struct device_node *child)
> +struct device_node *of_irq_find_parent(struct device_node *child)
>  {
> struct device_node *p;
> const __be32 *parp;
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 039f2eec49ce..0c9ea9fb5b63 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -93,6 +93,7 @@ static inline void of_msi_configure(struct device *dev, 
> struct device_node *np)
>   * so declare it here regardless of the CONFIG_OF_IRQ setting.
>   */
>  extern unsigned int irq_of_parse_and_map(struct device_node *node, int 
> index);
> +extern struct device_node *of_irq_find_parent(struct device_node *child);

This is the wrong place to add the prototype, and

>  u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 
> rid_in);
>
>  #else /* !CONFIG_OF && !CONFIG_SPARC */
> @@ -102,6 +103,11 @@ static inline unsigned int irq_of_parse_and_map(struct 
> device_node *dev,
> return 0;
>  }
>
> +static inline void *of_irq_find_parent(struct device_node *child)
> +{
> +   return NULL;
> +}
> +

This is the wrong place to add the inline version. Both need to be
within the #ifdef CONFIG_OF_IRQ ... #else ... #endif block, as the
function is not available just with CONFIG_OF.


>  static inline u32 of_msi_map_rid(struct device *dev,
>  struct device_node *msi_np, u32 rid_in)

@Daney:

And this one is at the wrong place as well. Please fix it.

This will break the build if CONFIG_OF is enabled but CONFIG_OF_IRQ
isn't, and something tries to call these functions.


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

Re: [PATCH] Revert "of/irq: make of_irq_find_parent static"

[Jonas Gorski]

Hi,

On Wed, Dec 2, 2015 at 5:14 PM, Qais Yousef  wrote:
> This reverts commit 52493d446141b07c8ba28dd6a529513f8b2342bd.
>
> Signed-off-by: Qais Yousef 
>
> Conflicts:
> include/linux/of_irq.h
> ---
> I have a patch series that is under review that makes use of 
> of_irq_find_parent()
>
> The affected patch is this:
>
> https://lkml.org/lkml/2015/11/25/291
>
> Is it wrong to use this function? If yes, what's the alternative?
> If no, OK to revert?

Make the revert part of the series that needs it, so it won't get
moved again because of no external users.

also ...

>
> Thanks,
> Qais
>
>
>  drivers/of/irq.c   | 2 +-
>  include/linux/of_irq.h | 6 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 902b89be7217..45735d56e435 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -53,7 +53,7 @@ EXPORT_SYMBOL_GPL(irq_of_parse_and_map);
>   * Returns a pointer to the interrupt parent node, or NULL if the interrupt
>   * parent could not be determined.
>   */
> -static struct device_node *of_irq_find_parent(struct device_node *child)
> +struct device_node *of_irq_find_parent(struct device_node *child)
>  {
> struct device_node *p;
> const __be32 *parp;
> diff --git a/include/linux/of_irq.h b/include/linux/of_irq.h
> index 039f2eec49ce..0c9ea9fb5b63 100644
> --- a/include/linux/of_irq.h
> +++ b/include/linux/of_irq.h
> @@ -93,6 +93,7 @@ static inline void of_msi_configure(struct device *dev, 
> struct device_node *np)
>   * so declare it here regardless of the CONFIG_OF_IRQ setting.
>   */
>  extern unsigned int irq_of_parse_and_map(struct device_node *node, int 
> index);
> +extern struct device_node *of_irq_find_parent(struct device_node *child);

This is the wrong place to add the prototype, and

>  u32 of_msi_map_rid(struct device *dev, struct device_node *msi_np, u32 
> rid_in);
>
>  #else /* !CONFIG_OF && !CONFIG_SPARC */
> @@ -102,6 +103,11 @@ static inline unsigned int irq_of_parse_and_map(struct 
> device_node *dev,
> return 0;
>  }
>
> +static inline void *of_irq_find_parent(struct device_node *child)
> +{
> +   return NULL;
> +}
> +

This is the wrong place to add the inline version. Both need to be
within the #ifdef CONFIG_OF_IRQ ... #else ... #endif block, as the
function is not available just with CONFIG_OF.


>  static inline u32 of_msi_map_rid(struct device *dev,
>  struct device_node *msi_np, u32 rid_in)

@Daney:

And this one is at the wrong place as well. Please fix it.

This will break the build if CONFIG_OF is enabled but CONFIG_OF_IRQ
isn't, and something tries to call these functions.


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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
> On 01/12/15 10:41, Jonas Gorski wrote:
>> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
>>> +
>>> +   /* Go to start of buffer */
>>> +   buf -= FC_WORDS;
>>> +
>>> +   /* Erased if all data bytes are 0xFF */
>>> +   buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL;
>>> +
>>> +   if (!buf_erased)
>>> +   goto out_free;
>>
>> We now have a function exactly for that use case in 4.4,
>> nand_check_erased_buf [1], consider using that. This also has the
>> benefit of treating bit flips as correctable as long as the ECC scheme
>> is strong enough.
>
> I have no idea whether or not it's appropriate to specify
> bitflips_threshold > 0 so it'd just be a more complex way to do
> a memchr_inv() search for 0xFF.

The threshold would be the amount of bitflips the code can correct, so
basically ecc.strength (at least that is my understanding).

> The code also has to check for the hamming code bytes being all 0x00,
> because according to the comments [2], the controller also has
> difficulty with the non-erased all-0xFFs scenario too.

According to brcmnand.c hamming can fix up to fifteen bitflips, but in
the current code you would fail a hamming protected all-0xff-page for
even a single bitflip in the data or in the ecc bytes, which means
that all-0xff-pages wouldn't be protected at all.


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

Re: [PATCH (v6) 1/2] mtd: brcmnand: Add brcm,bcm63268-nand device tree binding

[Jonas Gorski]

On Wed, Dec 2, 2015 at 8:05 PM, Brian Norris
 wrote:
> + Broadcom list + Kamal
>
> On Tue, Nov 24, 2015 at 08:19:37PM -, Simon Arlott wrote:
>> Add device tree binding for NAND on the BCM63268.
>>
>> The BCM63268 has a NAND interrupt register with combined status and enable
>> registers.
>>
>> Signed-off-by: Simon Arlott 
>> ---
>>  .../devicetree/bindings/mtd/brcm,brcmnand.txt  | 35 
>> ++
>>  1 file changed, 35 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> index 4ff7128..f2a71c8 100644
>> --- a/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>> @@ -72,6 +72,14 @@ we define additional 'compatible' properties and 
>> associated register resources w
>> and enable registers
>>   - reg-names: (required) "nand-int-base"
>>
>> +   * "brcm,nand-bcm63268"
>> + - compatible: should contain "brcm,nand-bcm", "brcm,nand-bcm63268"
>
> Looks like you're aiming to support bcm63168? Is bcm63268 the first
> chip to include this style of register then? The numbering seems
> backwards, but that may just be reality.

There are four chip variants, bcm63168, bcm63268, bcm63169 and
bcm63269, and they were all introduced at the same time. the *16x have
less features than *26x (IIRC only two rgmii ports instead of four, no
hw crypto, maybe no dect?), and the *9 ones have no xDSL support.

>From a registers location/layout, clocks, etc standpoint, there is no
difference between bcm63168 and bcm63268 (and the x9's).

As a reference, Broadcom uses bcm63268 as the "generic" name for the
chip family in their code, but on their website only advertise the
bcm63168. Both chips do exist though, and at least the bcm63169, I
have devices with these three here.


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

Re: [PATCH] mtd: brcmnand: Workaround false ECC uncorrectable errors

[Jonas Gorski]

Hi,

On Wed, Dec 2, 2015 at 9:54 PM, Brian Norris
<computersforpe...@gmail.com> wrote:
> Hi,
>
> On Wed, Dec 02, 2015 at 09:44:04PM +0100, Jonas Gorski wrote:
>> On Wed, Dec 2, 2015 at 9:17 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
>> > On 01/12/15 10:41, Jonas Gorski wrote:
>> >> On Sat, Nov 28, 2015 at 8:23 PM, Simon Arlott <si...@fire.lp0.eu> wrote:
>> >>> +
>> >>> +   /* Go to start of buffer */
>> >>> +   buf -= FC_WORDS;
>> >>> +
>> >>> +   /* Erased if all data bytes are 0xFF */
>> >>> +   buf_erased = memchr_inv(buf, 0xFF, FC_WORDS) == NULL;
>> >>> +
>> >>> +   if (!buf_erased)
>> >>> +   goto out_free;
>> >>
>> >> We now have a function exactly for that use case in 4.4,
>> >> nand_check_erased_buf [1], consider using that. This also has the
>> >> benefit of treating bit flips as correctable as long as the ECC scheme
>> >> is strong enough.
>> >
>> > I have no idea whether or not it's appropriate to specify
>> > bitflips_threshold > 0 so it'd just be a more complex way to do
>> > a memchr_inv() search for 0xFF.
>>
>> The threshold would be the amount of bitflips the code can correct, so
>> basically ecc.strength (at least that is my understanding).
>>
>> > The code also has to check for the hamming code bytes being all 0x00,
>> > because according to the comments [2], the controller also has
>> > difficulty with the non-erased all-0xFFs scenario too.
>>
>> According to brcmnand.c hamming can fix up to fifteen bitflips, but in
>
> Hamming only protects 1 bitflip. The '15' is the value used by the
> controller to represent Hamming (i.e., there is no BCH-15).

Ah, yeah that confused me because I also vaguely remembered hamming
only providing protection for 1, but then saw the ecc_level = 15
assignment.

Still, that means that even hamming protected erased pages with a
single bitflip should be treated as readable / all-0xff, but with
correctable bitflips, and not as uncorrectable.

>> the current code you would fail a hamming protected all-0xff-page for
>> even a single bitflip in the data or in the ecc bytes, which means
>> that all-0xff-pages wouldn't be protected at all.
>
> BTW, I think Kamal had code to handle protecting bitflips in erased
> pages code in the Broadcom STB Linux BSP. Perhaps he can port that to
> upstream with nand_check_erased_ecc_chunk()? IIUC, that would probably
> handle your case too, Simon, although it wouldn't be optimal for an
> all-0xff check (i.e., bitflip_threshold == 0).
>
> If that's really an issue (i.e., we have an implementation + data), I'm
> sure we could add optimization to nand_check_erased_ecc_chunk() to
> support the bitflip_threshold == 0 case.

Maybe I'm missing something, but wasn't the point of introducing
nand_check_erased_ecc_chunk that bitflips in erased pages should be
treated as bitflips corrected by the ecc, and therefore fixed up
before passing the data further on? So having a theshold of 0 would be
wrong / no protection at all, and could be quite destructive on MLC
nand, where bitflips in erased pages are rather common.


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