Discussion:
[PATCH 02/12] ARM: ep93xx: use ARM_PATCH_PHYS_VIRT
Arnd Bergmann
2017-07-20 15:49:27 UTC
Permalink
Just like ARCH_MULTIPLATFORM, we want to use ARM_PATCH_PHYS_VIRT
when possible, but that fails for NOMMU or XIP_KERNEL configurations.
Using 'imply' instead of 'select' gets this right and only uses
the symbol when we don't have to hardcode the address anyway.

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
arch/arm/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d6e6f40addf6..db856355bd24 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -406,7 +406,7 @@ config ARCH_EP93XX
bool "EP93xx-based"
select ARCH_HAS_HOLES_MEMORYMODEL
select ARM_AMBA
- select ARM_PATCH_PHYS_VIRT
+ imply ARM_PATCH_PHYS_VIRT
select ARM_VIC
select AUTO_ZRELADDR
select CLKDEV_LOOKUP
--
2.9.0
Arnd Bergmann
2017-07-20 15:50:46 UTC
Permalink
Six ARM platforms still provide their own variant of the clk API
rather than using the generic COMMON_CLK API. This generally works,
but it causes some link errors with drivers using the clk_set_rate,
clk_get_parent, clk_set_parent or clk_round_rate functions when
a platform lacks those interfaces.

OMAP1 implements the whole set already, but davinci, ep93xx, mmp,
sa1100 and w90x900 all lack at least one of the functions.

This adds empty stub implementations for each of them, and I
don't even try to do something useful here but instead just
print a WARN() message to make it obvious what is going on
if they ever end up being called.

The drivers that call these won't be used on these platforms
(otherwise we'd get a link error today), so the added code is harmless
bloat and will warn about accidental use.

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
arch/arm/mach-davinci/clock.c | 6 ++++++
arch/arm/mach-ep93xx/clock.c | 21 +++++++++++++++++++++
arch/arm/mach-mmp/clock.c | 22 ++++++++++++++++++++++
arch/arm/mach-sa1100/clock.c | 29 +++++++++++++++++++++++++++++
arch/arm/mach-w90x900/clock.c | 29 +++++++++++++++++++++++++++++
5 files changed, 107 insertions(+)

diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index f5dce9b4e617..89586779526c 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -218,6 +218,12 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
}
EXPORT_SYMBOL(clk_set_parent);

+struct clk *clk_get_parent(struct clk *clk)
+{
+ return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
int clk_register(struct clk *clk)
{
if (clk == NULL || IS_ERR(clk))
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..c1c91fc6e178 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -323,6 +323,27 @@ unsigned long clk_get_rate(struct clk *clk)
}
EXPORT_SYMBOL(clk_get_rate);

+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
static int set_keytchclk_rate(struct clk *clk, unsigned long rate)
{
u32 val;
diff --git a/arch/arm/mach-mmp/clock.c b/arch/arm/mach-mmp/clock.c
index 28fe64c6e2f5..78513f74ae8d 100644
--- a/arch/arm/mach-mmp/clock.c
+++ b/arch/arm/mach-mmp/clock.c
@@ -106,3 +106,25 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
return ret;
}
EXPORT_SYMBOL(clk_set_rate);
+
+/* Dummy clk routine to build generic kernel parts that may be using them */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index 0db46895c82a..a602d876c231 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -35,6 +35,35 @@ struct clk clk_##_name = { \

static DEFINE_SPINLOCK(clocks_lock);

+/* Dummy clk routine to build generic kernel parts that may be using them */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
static void clk_gpio27_enable(struct clk *clk)
{
/*
diff --git a/arch/arm/mach-w90x900/clock.c b/arch/arm/mach-w90x900/clock.c
index ac6fd1a2cb59..3f93fac98d97 100644
--- a/arch/arm/mach-w90x900/clock.c
+++ b/arch/arm/mach-w90x900/clock.c
@@ -93,3 +93,32 @@ void nuc900_subclk_enable(struct clk *clk, int enable)

__raw_writel(clken, W90X900_VA_CLKPWR + SUBCLK);
}
+
+/* dummy functions, should not be called */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
--
2.9.0
Alexander Sverdlin
2017-07-20 16:04:00 UTC
Permalink
Hello Arnd!

I'd like to insist on a functional implementation at least for EP93xx,
Post by Arnd Bergmann
Six ARM platforms still provide their own variant of the clk API
rather than using the generic COMMON_CLK API. This generally works,
but it causes some link errors with drivers using the clk_set_rate,
clk_get_parent, clk_set_parent or clk_round_rate functions when
a platform lacks those interfaces.
OMAP1 implements the whole set already, but davinci, ep93xx, mmp,
sa1100 and w90x900 all lack at least one of the functions.
This adds empty stub implementations for each of them, and I
don't even try to do something useful here but instead just
print a WARN() message to make it obvious what is going on
if they ever end up being called.
The drivers that call these won't be used on these platforms
(otherwise we'd get a link error today), so the added code is harmless
bloat and will warn about accidental use.
---
arch/arm/mach-davinci/clock.c | 6 ++++++
arch/arm/mach-ep93xx/clock.c | 21 +++++++++++++++++++++
arch/arm/mach-mmp/clock.c | 22 ++++++++++++++++++++++
arch/arm/mach-sa1100/clock.c | 29 +++++++++++++++++++++++++++++
arch/arm/mach-w90x900/clock.c | 29 +++++++++++++++++++++++++++++
5 files changed, 107 insertions(+)
diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index f5dce9b4e617..89586779526c 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -218,6 +218,12 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
}
EXPORT_SYMBOL(clk_set_parent);
+struct clk *clk_get_parent(struct clk *clk)
+{
+ return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
int clk_register(struct clk *clk)
{
if (clk == NULL || IS_ERR(clk))
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..c1c91fc6e178 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -323,6 +323,27 @@ unsigned long clk_get_rate(struct clk *clk)
}
EXPORT_SYMBOL(clk_get_rate);
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
I've sent the patch implementing this properly several times already
(the last one "[PATCH v5 1/5] clk: ep93xx: Implement clk_get_parent()"
on 04.06.2017) and this is really required for the ADC driver which
comes along with the series.

I don't mind if you take mine or implement it within your patch,
but I'd like to see the working function merged.
Post by Arnd Bergmann
static int set_keytchclk_rate(struct clk *clk, unsigned long rate)
{
u32 val;
diff --git a/arch/arm/mach-mmp/clock.c b/arch/arm/mach-mmp/clock.c
index 28fe64c6e2f5..78513f74ae8d 100644
--- a/arch/arm/mach-mmp/clock.c
+++ b/arch/arm/mach-mmp/clock.c
@@ -106,3 +106,25 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
return ret;
}
EXPORT_SYMBOL(clk_set_rate);
+
+/* Dummy clk routine to build generic kernel parts that may be using them */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
diff --git a/arch/arm/mach-sa1100/clock.c b/arch/arm/mach-sa1100/clock.c
index 0db46895c82a..a602d876c231 100644
--- a/arch/arm/mach-sa1100/clock.c
+++ b/arch/arm/mach-sa1100/clock.c
@@ -35,6 +35,35 @@ struct clk clk_##_name = { \
static DEFINE_SPINLOCK(clocks_lock);
+/* Dummy clk routine to build generic kernel parts that may be using them */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
static void clk_gpio27_enable(struct clk *clk)
{
/*
diff --git a/arch/arm/mach-w90x900/clock.c b/arch/arm/mach-w90x900/clock.c
index ac6fd1a2cb59..3f93fac98d97 100644
--- a/arch/arm/mach-w90x900/clock.c
+++ b/arch/arm/mach-w90x900/clock.c
@@ -93,3 +93,32 @@ void nuc900_subclk_enable(struct clk *clk, int enable)
__raw_writel(clken, W90X900_VA_CLKPWR + SUBCLK);
}
+
+/* dummy functions, should not be called */
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
+
+int clk_set_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_rate);
+
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+ WARN_ON(clk);
+ return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
--
Best regards,
Alexander Sverdlin.
Arnd Bergmann
2017-07-21 07:51:38 UTC
Permalink
On Thu, Jul 20, 2017 at 6:04 PM, Alexander Sverdlin
Post by Alexander Sverdlin
I've sent the patch implementing this properly several times already
(the last one "[PATCH v5 1/5] clk: ep93xx: Implement clk_get_parent()"
on 04.06.2017) and this is really required for the ADC driver which
comes along with the series.
I don't mind if you take mine or implement it within your patch,
but I'd like to see the working function merged.
Hi Alexander,

Sorry for missing your patch. Of course we should just take that version
then. I'll drop the ep93xx part of my patch. When you send your version
for inclusion, please also add a clk_set_parent()/clk_get_parent()
implementation doing whatever you feel is right.

Arnd
Sekhar Nori
2017-07-21 08:05:51 UTC
Permalink
Hi Arnd,
Post by Arnd Bergmann
Six ARM platforms still provide their own variant of the clk API
rather than using the generic COMMON_CLK API. This generally works,
but it causes some link errors with drivers using the clk_set_rate,
clk_get_parent, clk_set_parent or clk_round_rate functions when
a platform lacks those interfaces.
OMAP1 implements the whole set already, but davinci, ep93xx, mmp,
sa1100 and w90x900 all lack at least one of the functions.
This adds empty stub implementations for each of them, and I
don't even try to do something useful here but instead just
print a WARN() message to make it obvious what is going on
if they ever end up being called.
The drivers that call these won't be used on these platforms
(otherwise we'd get a link error today), so the added code is harmless
bloat and will warn about accidental use.
---
arch/arm/mach-davinci/clock.c | 6 ++++++
arch/arm/mach-ep93xx/clock.c | 21 +++++++++++++++++++++
arch/arm/mach-mmp/clock.c | 22 ++++++++++++++++++++++
arch/arm/mach-sa1100/clock.c | 29 +++++++++++++++++++++++++++++
arch/arm/mach-w90x900/clock.c | 29 +++++++++++++++++++++++++++++
5 files changed, 107 insertions(+)
diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index f5dce9b4e617..89586779526c 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -218,6 +218,12 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
}
EXPORT_SYMBOL(clk_set_parent);
+struct clk *clk_get_parent(struct clk *clk)
+{
+ return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
int clk_register(struct clk *clk)
{
if (clk == NULL || IS_ERR(clk))
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..c1c91fc6e178 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -323,6 +323,27 @@ unsigned long clk_get_rate(struct clk *clk)
}
EXPORT_SYMBOL(clk_get_rate);
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
Its probably better to WARN_ON_ONCE(). Also, since NULL clk is valid, it
should be probably be WARN_ON_ONCE(1).

Thanks,
Sekhar
Arnd Bergmann
2017-07-21 08:17:13 UTC
Permalink
Post by Sekhar Nori
Post by Arnd Bergmann
diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index f5dce9b4e617..89586779526c 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -218,6 +218,12 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
}
EXPORT_SYMBOL(clk_set_parent);
+struct clk *clk_get_parent(struct clk *clk)
+{
+ return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
Thanks!
Post by Sekhar Nori
Post by Arnd Bergmann
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..c1c91fc6e178 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -323,6 +323,27 @@ unsigned long clk_get_rate(struct clk *clk)
}
EXPORT_SYMBOL(clk_get_rate);
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
Its probably better to WARN_ON_ONCE(). Also, since NULL clk is valid, it
should be probably be WARN_ON_ONCE(1).
I thought about trying to make it as correct as possible, but then went for
the opposite approach and instead just made the functions as small as
possible to avoid bloat while having the warning in there along with an
obviously wrong implementation. Apparently the davinci version unlike
the others ended up being a correct one after all (let me know if you
want to have a NULL pointer check there).

I also remembered now that the mmp implementation is actually dead
code (we always use the COMMON_CLK implementation for mmp
these days). I think I'll just drop the mmp patch, and split out davinci,
w90x900 and sa1100 into separate patches now, as there are only
three of them left.

Arnd
Sekhar Nori
2017-07-21 09:44:46 UTC
Permalink
Post by Arnd Bergmann
Post by Sekhar Nori
Post by Arnd Bergmann
diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index f5dce9b4e617..89586779526c 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -218,6 +218,12 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
}
EXPORT_SYMBOL(clk_set_parent);
+struct clk *clk_get_parent(struct clk *clk)
+{
+ return clk->parent;
+}
+EXPORT_SYMBOL(clk_get_parent);
Thanks!
Post by Sekhar Nori
Post by Arnd Bergmann
diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 39ef3b613912..c1c91fc6e178 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -323,6 +323,27 @@ unsigned long clk_get_rate(struct clk *clk)
}
EXPORT_SYMBOL(clk_get_rate);
+long clk_round_rate(struct clk *clk, unsigned long rate)
+{
+ WARN_ON(clk);
+ return 0;
+}
+EXPORT_SYMBOL(clk_round_rate);
Its probably better to WARN_ON_ONCE(). Also, since NULL clk is valid, it
should be probably be WARN_ON_ONCE(1).
I thought about trying to make it as correct as possible, but then went for
the opposite approach and instead just made the functions as small as
possible to avoid bloat while having the warning in there along with an
obviously wrong implementation. Apparently the davinci version unlike
the others ended up being a correct one after all (let me know if you
want to have a NULL pointer check there).
Yeah, that would be better. Thanks!

Regards,
Sekhar
Arnd Bergmann
2017-07-20 15:45:55 UTC
Permalink
An empty macro definition can cause unexpected behavior, in
case of the ixp4xx ioport_unmap, we get two warnings:

drivers/net/wireless/marvell/libertas/if_cs.c: In function 'if_cs_release':
drivers/net/wireless/marvell/libertas/if_cs.c:826:3: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
ioport_unmap(card->iobase);
drivers/vfio/pci/vfio_pci_rdwr.c: In function 'vfio_pci_vga_rw':
drivers/vfio/pci/vfio_pci_rdwr.c:230:15: error: the omitted middle operand in ?: will always be 'true', suggest explicit middle operand [-Werror=parentheses]
is_ioport ? ioport_unmap(iomem) : iounmap(iomem);

This uses an inline function to define the macro in a safer way.

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
arch/arm/mach-ixp4xx/include/mach/io.h | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h
index 7a0c13bf4269..2f84c26a6758 100644
--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -523,8 +523,15 @@ static inline void iowrite32_rep(void __iomem *addr, const void *vaddr,
#endif
}

-#define ioport_map(port, nr) ((void __iomem*)(port + PIO_OFFSET))
-#define ioport_unmap(addr)
+#define ioport_map(port, nr) ioport_map(port, nr)
+static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
+{
+ return ((void __iomem*)((port) + PIO_OFFSET));
+}
+#define ioport_unmap(addr) ioport_unmap(addr)
+static inline void ioport_unmap(void __iomem *addr)
+{
+}
#endif /* CONFIG_PCI */

#endif /* __ASM_ARM_ARCH_IO_H */
--
2.9.0
Krzysztof Hałasa
2017-07-21 12:21:53 UTC
Permalink
Post by Arnd Bergmann
An empty macro definition can cause unexpected behavior, in
drivers/net/wireless/marvell/libertas/if_cs.c:826:3: error: suggest braces around empty body in an 'if' statement [-Werror=empty-body]
ioport_unmap(card->iobase);
drivers/vfio/pci/vfio_pci_rdwr.c:230:15: error: the omitted middle operand in ?: will always be 'true', suggest explicit middle operand [-Werror=parentheses]
is_ioport ? ioport_unmap(iomem) : iounmap(iomem);
This uses an inline function to define the macro in a safer way.
---
arch/arm/mach-ixp4xx/include/mach/io.h | 11 +++++++++--
--- a/arch/arm/mach-ixp4xx/include/mach/io.h
-#define ioport_map(port, nr) ((void __iomem*)(port + PIO_OFFSET))
^^^^
--
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Arnd Bergmann
2017-07-20 15:51:42 UTC
Permalink
In commit 3169663ac5902 "ARM: sa11x0/pxa: convert OS timer registers
to IOMEM", the definition of the OSCR macro was changed to be an
__iomem pointer, but the same register is also used by the XIP
code. This patch does the corresponding change here as well.

On PXA, the IRQ register definitions were removed even earlier, in
commit 5d284e353eb1 ("ARM: pxa: avoid accessing interrupt registers
directly"). This patch unfortunately brings some of that back. An
earlier version of my patch moved the code into an external function,
which could not work for CONFIG_XIP_KERNEL+CONFIG_MTD_XIP, so this
restores something close to the original code.

Link: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-March/241716.html
Signed-off-by: Arnd Bergmann <***@arndb.de>
Cc: Russell King <***@arm.linux.org.uk>
---
---
arch/arm/mach-pxa/include/mach/mtd-xip.h | 10 +++++++---
arch/arm/mach-sa1100/include/mach/mtd-xip.h | 4 ++--
2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-pxa/include/mach/mtd-xip.h b/arch/arm/mach-pxa/include/mach/mtd-xip.h
index 990d2bf2fb45..f0d40bd36bac 100644
--- a/arch/arm/mach-pxa/include/mach/mtd-xip.h
+++ b/arch/arm/mach-pxa/include/mach/mtd-xip.h
@@ -17,11 +17,15 @@

#include <mach/regs-ost.h>

-#define xip_irqpending() (ICIP & ICMR)
+/* restored July 2017, this did not build since 2011! */
+
+#define ICIP io_p2v(0x40d00000)
+#define ICMR io_p2v(0x40d00004)
+#define xip_irqpending() readl(ICIP) & readl(ICMR)

/* we sample OSCR and convert desired delta to usec (1/4 ~= 1000000/3686400) */
-#define xip_currtime() (OSCR)
-#define xip_elapsed_since(x) (signed)((OSCR - (x)) / 4)
+#define xip_currtime() readl(OSCR)
+#define xip_elapsed_since(x) (signed)((readl(OSCR) - (x)) / 4)

/*
* xip_cpu_idle() is used when waiting for a delay equal or larger than
diff --git a/arch/arm/mach-sa1100/include/mach/mtd-xip.h b/arch/arm/mach-sa1100/include/mach/mtd-xip.h
index b3d684098fbf..cb76096a2e36 100644
--- a/arch/arm/mach-sa1100/include/mach/mtd-xip.h
+++ b/arch/arm/mach-sa1100/include/mach/mtd-xip.h
@@ -20,7 +20,7 @@
#define xip_irqpending() (ICIP & ICMR)

/* we sample OSCR and convert desired delta to usec (1/4 ~= 1000000/3686400) */
-#define xip_currtime() (OSCR)
-#define xip_elapsed_since(x) (signed)((OSCR - (x)) / 4)
+#define xip_currtime() readl_relaxed(OSCR)
+#define xip_elapsed_since(x) (signed)((readl_relaxed(OSCR) - (x)) / 4)

#endif /* __ARCH_SA1100_MTD_XIP_H__ */
--
2.9.0
Robert Jarzmik
2017-07-23 15:53:03 UTC
Permalink
Arnd Bergmann <***@arndb.de> writes:

Hi Arnd,
Post by Arnd Bergmann
-#define xip_irqpending() (ICIP & ICMR)
+/* restored July 2017, this did not build since 2011! */
+
+#define ICIP io_p2v(0x40d00000)
+#define ICMR io_p2v(0x40d00004)
Okay, I suppose the IO mapping is guaranteed to work, ie. io_p2v() is behaving
correctly when xip_irqpending() is used, right ? I'm not challenging this, I'm
just ensuring this _could_ work (if anybody had the silly idea to make it work
again, and I admit I don't have that much courage).
Post by Arnd Bergmann
+#define xip_irqpending() readl(ICIP) & readl(ICMR)
This is not strictly equivalent to (ICIP & ICMR), I would have put for priority
reasons :
+#define xip_irqpending() (readl(ICIP) & readl(ICMR))

..zip..

Cheers.
--
Robert
Arnd Bergmann
2017-07-25 15:14:57 UTC
Permalink
Post by Sekhar Nori
Hi Arnd,
-#define xip_irqpending() (ICIP & ICMR)
+/* restored July 2017, this did not build since 2011! */
+
+#define ICIP io_p2v(0x40d00000)
+#define ICMR io_p2v(0x40d00004)
Okay, I suppose the IO mapping is guaranteed to work, ie. io_p2v() is behaving
correctly when xip_irqpending() is used, right ? I'm not challenging this, I'm
just ensuring this _could_ work (if anybody had the silly idea to make it work
again, and I admit I don't have that much courage).
Good thinking ;-)

I double-checked this and found that it is correct, and we still map that memory
in the same place in pxa_map_io().
Post by Sekhar Nori
+#define xip_irqpending() readl(ICIP) & readl(ICMR)
This is not strictly equivalent to (ICIP & ICMR), I would have put for priority
+#define xip_irqpending() (readl(ICIP) & readl(ICMR))
Ok, I've changed that and will resend.

Arnd

Arnd Bergmann
2017-07-20 15:51:43 UTC
Permalink
This variable may be used by some devices that each have their
on Kconfig symbol, or by none of them, and that causes a build
warning:

arch/arm/mach-mmp/devices.c:241:12: error: 'usb_dma_mask' defined but not used [-Werror=unused-variable]

Marking it __maybe_unused avoids the warning.

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
Originally sent in Feb 2016, this went missing for some reason.
---
arch/arm/mach-mmp/devices.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-mmp/devices.c b/arch/arm/mach-mmp/devices.c
index 3330ac7cfbef..671c7a09ab3d 100644
--- a/arch/arm/mach-mmp/devices.c
+++ b/arch/arm/mach-mmp/devices.c
@@ -238,7 +238,7 @@ void pxa_usb_phy_deinit(void __iomem *phy_reg)
#endif

#if IS_ENABLED(CONFIG_USB_SUPPORT)
-static u64 usb_dma_mask = ~(u32)0;
+static u64 __maybe_unused usb_dma_mask = ~(u32)0;

#if IS_ENABLED(CONFIG_USB_MV_UDC)
struct resource pxa168_u2o_resources[] = {
--
2.9.0
Arnd Bergmann
2017-07-20 15:51:46 UTC
Permalink
sirfsoc_init_late is called by each of the three individual
SoC definitions, but in a randconfig build, we can encounter
a situation where they are all disabled:

arch/arm/mach-prima2/common.c:18:123: warning: 'sirfsoc_init_late' defined but not used [-Wunused-function]

While that is not a useful configuration, the warning also
doesn't help, so this patch marks the function as __maybe_unused
to let the compiler know it is there intentionally.

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
Originally submitted Feb 2016, but it got lost
---
arch/arm/mach-prima2/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-prima2/common.c b/arch/arm/mach-prima2/common.c
index 8cadb302a7d2..ffe05c27087e 100644
--- a/arch/arm/mach-prima2/common.c
+++ b/arch/arm/mach-prima2/common.c
@@ -15,7 +15,7 @@
#include <linux/of_platform.h>
#include "common.h"

-static void __init sirfsoc_init_late(void)
+static void __init __maybe_unused sirfsoc_init_late(void)
{
sirfsoc_pm_init();
}
--
2.9.0
Arnd Bergmann
2017-07-20 15:53:43 UTC
Permalink
Bluetooth is only supported when network support is part of the kernel,
so it is a bit pointless to build the hi1940-bt support without networking.
If we try anyway, we get a Kconfig warning:

warning: (TOSA_BT && H1940BT) selects RFKILL which has unmet direct dependencies (NET)

This turns the 'select' into 'depends on' as Krzysztof suggested when
I first sent a fix.

Suggested-by: Krzysztof Kozlowski <***@kernel.org>
Link: https://patchwork.kernel.org/patch/8164161/
Signed-off-by: Arnd Bergmann <***@arndb.de>
---
v1 was sent in Jan 2016, and instead added a dependency on CONFIG_NET
---
arch/arm/mach-s3c24xx/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-s3c24xx/Kconfig b/arch/arm/mach-s3c24xx/Kconfig
index 648c519c7593..7da0165334ae 100644
--- a/arch/arm/mach-s3c24xx/Kconfig
+++ b/arch/arm/mach-s3c24xx/Kconfig
@@ -237,7 +237,7 @@ config ARCH_H1940
config H1940BT
tristate "Control the state of H1940 bluetooth chip"
depends on ARCH_H1940
- select RFKILL
+ depends on RFKILL
help
This is a simple driver that is able to control
the state of built in bluetooth chip on h1940.
--
2.9.0
Krzysztof Kozlowski
2017-07-20 19:30:38 UTC
Permalink
Post by Arnd Bergmann
Bluetooth is only supported when network support is part of the kernel,
so it is a bit pointless to build the hi1940-bt support without networking.
warning: (TOSA_BT && H1940BT) selects RFKILL which has unmet direct dependencies (NET)
This turns the 'select' into 'depends on' as Krzysztof suggested when
I first sent a fix.
Link: https://patchwork.kernel.org/patch/8164161/
---
v1 was sent in Jan 2016, and instead added a dependency on CONFIG_NET
---
arch/arm/mach-s3c24xx/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied.

Best regards,
Krzysztof
Arnd Bergmann
2017-07-20 15:51:44 UTC
Permalink
The omap_generic_init() and omap_hwmod_init_postsetup() functions are
used in the initialization for all OMAP2+ SoC types, but in the
extreme case that those are all disabled, we get a warning about
unused code:

arch/arm/mach-omap2/io.c:412:123: error: 'omap_hwmod_init_postsetup' defined but not used [-Werror=unused-function]
arch/arm/mach-omap2/board-generic.c:30:123: error: 'omap_generic_init' defined but not used [-Werror=unused-function]

This annotates both as __maybe_unused to shut up that warning.

Signed-off-by: Arnd Bergmann <***@arndb.de>
Acked-by: Tony Lindgren <***@atomide.com>
---
Originally sent in Feb 2016, but it got lost for some reason.
---
arch/arm/mach-omap2/board-generic.c | 2 +-
arch/arm/mach-omap2/io.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/board-generic.c b/arch/arm/mach-omap2/board-generic.c
index dc9e34e670a2..b1e661bb5521 100644
--- a/arch/arm/mach-omap2/board-generic.c
+++ b/arch/arm/mach-omap2/board-generic.c
@@ -28,7 +28,7 @@ static const struct of_device_id omap_dt_match_table[] __initconst = {
{ }
};

-static void __init omap_generic_init(void)
+static void __init __maybe_unused omap_generic_init(void)
{
pdata_quirks_init(omap_dt_match_table);

diff --git a/arch/arm/mach-omap2/io.c b/arch/arm/mach-omap2/io.c
index 1d739d1a0a65..1cd20e4d56b0 100644
--- a/arch/arm/mach-omap2/io.c
+++ b/arch/arm/mach-omap2/io.c
@@ -410,7 +410,7 @@ static int _set_hwmod_postsetup_state(struct omap_hwmod *oh, void *data)
return omap_hwmod_set_postsetup_state(oh, *(u8 *)data);
}

-static void __init omap_hwmod_init_postsetup(void)
+static void __init __maybe_unused omap_hwmod_init_postsetup(void)
{
u8 postsetup_state;
--
2.9.0
Arnd Bergmann
2017-07-20 15:51:47 UTC
Permalink
ixp4xx defines the arguments to its __indirect_writesb() and other
functions as pointers to fixed-size data. This is not necessarily
wrong, and it works most of the time, but it causes warnings in
at least one driver:

drivers/net/ethernet/smsc/smc91x.c: In function 'smc_rcv':
drivers/net/ethernet/smsc/smc91x.c:495:21: error: passing argument 2 of '__indirect_readsw' from incompatible pointer type [-Werror=incompatible-pointer-types]
SMC_PULL_DATA(lp, data, packet_len - 4);

All other definitions of the same functions pass void pointers,
so doing the same here avoids the warnings.

Signed-off-by: Arnd Bergmann <***@arndb.de>
Acked-by: Krzysztof Halasa <***@piap.pl>
---
Originally sent on Feb 2016, but it got lost
---
arch/arm/mach-ixp4xx/include/mach/io.h | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-ixp4xx/include/mach/io.h b/arch/arm/mach-ixp4xx/include/mach/io.h
index 2f84c26a6758..844e8ac593e2 100644
--- a/arch/arm/mach-ixp4xx/include/mach/io.h
+++ b/arch/arm/mach-ixp4xx/include/mach/io.h
@@ -95,8 +95,10 @@ static inline void __indirect_writeb(u8 value, volatile void __iomem *p)
}

static inline void __indirect_writesb(volatile void __iomem *bus_addr,
- const u8 *vaddr, int count)
+ const void *p, int count)
{
+ const u8 *vaddr = p;
+
while (count--)
writeb(*vaddr++, bus_addr);
}
@@ -118,8 +120,10 @@ static inline void __indirect_writew(u16 value, volatile void __iomem *p)
}

static inline void __indirect_writesw(volatile void __iomem *bus_addr,
- const u16 *vaddr, int count)
+ const void *p, int count)
{
+ const u16 *vaddr = p;
+
while (count--)
writew(*vaddr++, bus_addr);
}
@@ -137,8 +141,9 @@ static inline void __indirect_writel(u32 value, volatile void __iomem *p)
}

static inline void __indirect_writesl(volatile void __iomem *bus_addr,
- const u32 *vaddr, int count)
+ const void *p, int count)
{
+ const u32 *vaddr = p;
while (count--)
writel(*vaddr++, bus_addr);
}
@@ -160,8 +165,10 @@ static inline u8 __indirect_readb(const volatile void __iomem *p)
}

static inline void __indirect_readsb(const volatile void __iomem *bus_addr,
- u8 *vaddr, u32 count)
+ void *p, u32 count)
{
+ u8 *vaddr = p;
+
while (count--)
*vaddr++ = readb(bus_addr);
}
@@ -183,8 +190,10 @@ static inline u16 __indirect_readw(const volatile void __iomem *p)
}

static inline void __indirect_readsw(const volatile void __iomem *bus_addr,
- u16 *vaddr, u32 count)
+ void *p, u32 count)
{
+ u16 *vaddr = p;
+
while (count--)
*vaddr++ = readw(bus_addr);
}
@@ -204,8 +213,10 @@ static inline u32 __indirect_readl(const volatile void __iomem *p)
}

static inline void __indirect_readsl(const volatile void __iomem *bus_addr,
- u32 *vaddr, u32 count)
+ void *p, u32 count)
{
+ u32 *vaddr = p;
+
while (count--)
*vaddr++ = readl(bus_addr);
}
--
2.9.0
Arnd Bergmann
2017-07-20 15:53:44 UTC
Permalink
The modem pm handler in the ams-delta board uses regulator_enable()
but does not check for a successful return code:

board-ams-delta.c:521:3: error: ignoring return value of 'regulator_enable', declared with attribute warn_unused_result [-Werror=unused-result]

It is not easy to propagate that return code to the callers in
uart_configure_port/uart_suspend_port/uart_resume_port, unless
we change all UART drivers, and it is unclear what those would
do with the return code.

Instead, this patch uses a runtime warning to replace the
compiletime warning. I have checked that the regulator in question
is hardcoded to a fixed-voltage GPIO regulator, and that should
never fail to get enabled if I understand the code right.

Acked-by: Tony Lindgren <***@atomide.com>
Acked-by: Aaro Koskinen <***@iki.fi>
Link: https://patchwork.kernel.org/patch/8391981/
Signed-off-by: Arnd Bergmann <***@arndb.de>
---
Originally sent in Feb 2016, but it got lost for some reason
---
arch/arm/mach-omap1/board-ams-delta.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap1/board-ams-delta.c b/arch/arm/mach-omap1/board-ams-delta.c
index 6613a6ff5dbc..6cbc69c92913 100644
--- a/arch/arm/mach-omap1/board-ams-delta.c
+++ b/arch/arm/mach-omap1/board-ams-delta.c
@@ -510,6 +510,7 @@ static void __init ams_delta_init(void)
static void modem_pm(struct uart_port *port, unsigned int state, unsigned old)
{
struct modem_private_data *priv = port->private_data;
+ int ret;

if (IS_ERR(priv->regulator))
return;
@@ -518,9 +519,16 @@ static void modem_pm(struct uart_port *port, unsigned int state, unsigned old)
return;

if (state == 0)
- regulator_enable(priv->regulator);
+ ret = regulator_enable(priv->regulator);
else if (old == 0)
- regulator_disable(priv->regulator);
+ ret = regulator_disable(priv->regulator);
+ else
+ ret = 0;
+
+ if (ret)
+ dev_warn(port->dev,
+ "ams_delta modem_pm: failed to %sable regulator: %d\n",
+ state ? "dis" : "en", ret);
}

static struct plat_serial8250_port ams_delta_modem_ports[] = {
--
2.9.0
Arnd Bergmann
2017-07-20 15:51:45 UTC
Permalink
The osk_mistral_init() contains code that is only compiled when
CONFIG_PM is set, but it uses a variable that is declared outside
of the #ifdef:

arch/arm/mach-omap1/board-osk.c: In function 'osk_mistral_init':
arch/arm/mach-omap1/board-osk.c:513:7: warning: unused variable 'ret' [-Wunused-variable]

This removes the #ifdef around the user of the variable,
make it always used.

Signed-off-by: Arnd Bergmann <***@arndb.de>
Suggested-by: Tony Lindgren <***@atomide.com>
Acked-by: Aaro Koskinen <***@iki.fi>
---
arch/arm/mach-omap1/board-osk.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/arch/arm/mach-omap1/board-osk.c b/arch/arm/mach-omap1/board-osk.c
index 4dfb99504810..95ac1929aede 100644
--- a/arch/arm/mach-omap1/board-osk.c
+++ b/arch/arm/mach-omap1/board-osk.c
@@ -441,13 +441,11 @@ static struct spi_board_info __initdata mistral_boardinfo[] = { {
.chip_select = 0,
} };

-#ifdef CONFIG_PM
static irqreturn_t
osk_mistral_wake_interrupt(int irq, void *ignored)
{
return IRQ_HANDLED;
}
-#endif

static void __init osk_mistral_init(void)
{
@@ -515,7 +513,6 @@ static void __init osk_mistral_init(void)

gpio_direction_input(OMAP_MPUIO(2));
irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
-#ifdef CONFIG_PM
/* share the IRQ in case someone wants to use the
* button for more than wakeup from system sleep.
*/
@@ -529,7 +526,6 @@ static void __init osk_mistral_init(void)
ret);
} else
enable_irq_wake(irq);
-#endif
} else
printk(KERN_ERR "OSK+Mistral: wakeup button is awol\n");
--
2.9.0
Arnd Bergmann
2017-07-20 15:53:45 UTC
Permalink
The RAM_SIZE macro in mach/hardware.h conflicts with macros of
the same name in multiple drivers, leading to annoying build warnings:

In file included from drivers/net/ethernet/cirrus/cs89x0.c:79:0:
drivers/net/ethernet/cirrus/cs89x0.h:324:0: error: "RAM_SIZE" redefined [-Werror]
#define RAM_SIZE 0x1000 /* The card has 4k bytes or RAM */
^
In file included from /git/arm-soc/arch/arm/mach-rpc/include/mach/io.h:16:0,
from /git/arm-soc/arch/arm/include/asm/io.h:194,
from /git/arm-soc/include/linux/scatterlist.h:8,
from /git/arm-soc/include/linux/dmaengine.h:24,
from /git/arm-soc/include/linux/netdevice.h:38,
from /git/arm-soc/drivers/net/ethernet/cirrus/cs89x0.c:54:
arch/arm/mach-rpc/include/mach/hardware.h:28:0: note: this is the location of the previous definition
#define RAM_SIZE 0x10000000

We don't use RAM_SIZE/RAM_START at all, so we could just remove
them, but it might be nice to leave them for documentation purposes,
so this renames them to RPC_RAM_SIZE/RPC_RAM_START in order to
avoid the build warnings

Signed-off-by: Arnd Bergmann <***@arndb.de>
---
I originally sent this in Feb 2016, but it got lost for some reason.
---
arch/arm/mach-rpc/include/mach/hardware.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-rpc/include/mach/hardware.h b/arch/arm/mach-rpc/include/mach/hardware.h
index aa79fa47373a..622d4e5df029 100644
--- a/arch/arm/mach-rpc/include/mach/hardware.h
+++ b/arch/arm/mach-rpc/include/mach/hardware.h
@@ -25,8 +25,8 @@
* *_SIZE is the size of the region
* *_BASE is the virtual address
*/
-#define RAM_SIZE 0x10000000
-#define RAM_START 0x10000000
+#define RPC_RAM_SIZE 0x10000000
+#define RPC_RAM_START 0x10000000

#define EASI_SIZE 0x08000000 /* EASI I/O */
#define EASI_START 0x08000000
--
2.9.0
Alexander Sverdlin
2017-07-20 16:07:53 UTC
Permalink
Post by Arnd Bergmann
Just like ARCH_MULTIPLATFORM, we want to use ARM_PATCH_PHYS_VIRT
when possible, but that fails for NOMMU or XIP_KERNEL configurations.
Using 'imply' instead of 'select' gets this right and only uses
the symbol when we don't have to hardcode the address anyway.
---
  arch/arm/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index d6e6f40addf6..db856355bd24 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -406,7 +406,7 @@ config ARCH_EP93XX
      bool "EP93xx-based"
      select ARCH_HAS_HOLES_MEMORYMODEL
      select ARM_AMBA
-    select ARM_PATCH_PHYS_VIRT
+    imply ARM_PATCH_PHYS_VIRT
      select ARM_VIC
      select AUTO_ZRELADDR
      select CLKDEV_LOOKUP
--
2.9.0
Loading...