[PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

This fixes the check for unused mdio bus setting and the following static
checker warning:
 drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
 warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)'

It also fixes the return var when calling of_get_child_count()

Reported-by: Dan Carpenter 
Signed-off-by: Phil Edworthy 
---
v2:
 - Don't rely on rely on the implicit typecast from -1 to uint
---
 drivers/pinctrl/pinctrl-rzn1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
index 57886dcff53d..cc0e5aa9128a 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -112,7 +112,7 @@ struct rzn1_pinctrl {
struct rzn1_pinctrl_regs __iomem *lev2;
u32 lev1_protect_phys;
u32 lev2_protect_phys;
-   u32 mdio_func[2];
+   int mdio_func[2];
 
struct rzn1_pin_group *groups;
unsigned int ngroups;
@@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device 
*pdev,
struct device_node *np = pdev->dev.of_node;
struct device_node *child;
unsigned int maxgroups = 0;
-   unsigned int nfuncs = 0;
unsigned int i = 0;
+   int nfuncs = 0;
int ret;
 
nfuncs = of_get_child_count(np);
-- 
2.17.1

[PATCH] clk: Add (devm_)clk_get_optional() functions

[Phil Edworthy]

This adds clk_get_optional() and devm_clk_get_optional() functions to get
optional clocks.
They behave the same as (devm_)clk_get except where there is no clock
producer. In this case, instead of returning -ENOENT, the function
returns NULL. This makes error checking simpler and allows
clk_prepare_enable, etc to be called on the returned reference
without additional checks.

Signed-off-by: Phil Edworthy 
---
v7:
 - Instead of messing with the core functions, simply wrap them for the
   _optional() versions. By putting clk_get_optional() inline in the header
   file, we can get rid of the arch specific patches as well.
v6:
 - Add doxygen style comment for devm_clk_get_optional() args
v5:
 - No changes.
v4:
 - No changes.
v3:
 - No changes.
---
 drivers/clk/clk-devres.c | 11 +++
 include/linux/clk.h  | 27 +++
 2 files changed, 38 insertions(+)

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 12c87457eca1..f0033d937c39 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -34,6 +34,17 @@ struct clk *devm_clk_get(struct device *dev, const char *id)
 }
 EXPORT_SYMBOL(devm_clk_get);
 
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+   struct clk *clk = devm_clk_get(dev, id);
+
+   if (clk == ERR_PTR(-ENOENT))
+   return NULL;
+   else
+   return clk;
+}
+EXPORT_SYMBOL(devm_clk_get_optional);
+
 struct clk_bulk_devres {
struct clk_bulk_data *clks;
int num_clks;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index a7773b5c0b9f..c7bbb0678057 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -383,6 +383,17 @@ int __must_check devm_clk_bulk_get_all(struct device *dev,
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
+/**
+ * devm_clk_get_optional - lookup and obtain a managed reference to an optional
+ *clock producer.
+ * @dev: device for clock "consumer"
+ * @id: clock consumer ID
+ *
+ * Behaves the same as devm_clk_get except where there is no clock producer. In
+ * this case, instead of returning -ENOENT, the function returns NULL.
+ */
+struct clk *devm_clk_get_optional(struct device *dev, const char *id);
+
 /**
  * devm_get_clk_from_child - lookup and obtain a managed reference to a
  *  clock producer from child node.
@@ -718,6 +729,12 @@ static inline struct clk *devm_clk_get(struct device *dev, 
const char *id)
return NULL;
 }
 
+static inline struct clk *devm_clk_get_optional(struct device *dev,
+   const char *id)
+{
+   return NULL;
+}
+
 static inline int __must_check devm_clk_bulk_get(struct device *dev, int 
num_clks,
 struct clk_bulk_data *clks)
 {
@@ -862,6 +879,16 @@ static inline void clk_bulk_disable_unprepare(int num_clks,
clk_bulk_unprepare(num_clks, clks);
 }
 
+static inline struct clk *clk_get_optional(struct device *dev, const char *id)
+{
+   struct clk *clk = clk_get(dev, id);
+
+   if (clk == ERR_PTR(-ENOENT))
+   return NULL;
+   else
+   return clk;
+}
+
 #if defined(CONFIG_OF) && defined(CONFIG_COMMON_CLK)
 struct clk *of_clk_get(struct device_node *np, int index);
 struct clk *of_clk_get_by_name(struct device_node *np, const char *name);
-- 
2.17.1

RE: [PATCH v6 1/6] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

Hi Uwe,

On 19 November 2018 12:58 Uwe Kleine-König wrote:
> On Mon, Nov 19, 2018 at 12:53:46PM +0000, Phil Edworthy wrote:
> > On 19 November 2018 10:46 Uwe Kleine-König wrote:
> > > On Mon, Nov 19, 2018 at 10:41:42AM +0000, Phil Edworthy wrote:
> > > > btw, do we need to add of_clk_get_by_name_optional()? I only added
> it
> > > > as a counterpart to of_clk_get_by_name(), but it may not be needed.
> > >
> > > I don't need it. Given that it is easy to add when someone has a need, I'd
> say,
> > > skip it for now.
> >
> > I'm wondering if we actually need clk_get_optional(). For me at least, I 
> > just
> > want devm_clk_get_optional(). That would get rid of the arch patches.
> 
> Given that clk_get_optional will be that simple, it can live in
> linux/clk.h for all implementors of the clk API, then you don't have to
> care about different archs. (Unless I'm missing something.)
You are absolutely right, I'm such a clutz sometimes!

Thanks
Phil

> I don't think it's a good idea to drop clk_get_optional even if you'd
> have to provide arch-specific stuff.
> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.   | Uwe Kleine-König|
> Industrial Linux Solutions | http://www.pengutronix.de/  |

RE: [PATCH v6 1/6] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

Hi Uwe,

On 19 November 2018 10:46 Uwe Kleine-König wrote:
> On Mon, Nov 19, 2018 at 10:41:42AM +0000, Phil Edworthy wrote:
> > On 16 November 2018 16:11 Uwe Kleine-König wrote:
> > > On Fri, Nov 16, 2018 at 05:01:28PM +0100, Uwe Kleine-König wrote:
> > > > Other than that I think the patch is fine
> > >
> > > Thinking again, I wonder why not just do:
> > >
> > > static inline struct clk *clk_get_optional(struct device *dev, const char
> *id) {
> > >   struct clk *c = clk_get(dev, id);
> > >
> > >   if (c == ERR_PTR(-ENOENT))
> > >   return NULL;
> > >   else
> > >   return c;
> > > }
> >
> > Unfortunately, underneath this __of_clk_get_by_name() returns -EINVAL
> > when looking for a named clock, and the "clock-names" OF property
> > can't be found or the name is not in that prop. This is because the
> > index returned by of_property_match_string() will be an error code and
> > is then currently always passed to __of_clk_get().
> >
> > If, as you said, I split the patches into one that fixes the error
> > code, and then adds clk_get_optional() like above, it will make more sense.
> 
> Sounds like a good plan.
Now that I have removed of_clk_get_by_name_optional(), I see that clk_get()
deals with __of_clk_get_by_name() returning -EINVAL and -ENOENT the same
way. In both cases, clk_get_sys() will return -ENOENT... i.e. I no longer need 
to
modify __of_clk_get_by_name().
All I need is a simple wrapper just as you have outlined above.

> > btw, do we need to add of_clk_get_by_name_optional()? I only added it
> > as a counterpart to of_clk_get_by_name(), but it may not be needed.
> 
> I don't need it. Given that it is easy to add when someone has a need, I'd 
> say,
> skip it for now.
I'm wondering if we actually need clk_get_optional(). For me at least, I just
want devm_clk_get_optional(). That would get rid of the arch patches.

Thanks
Phil

RE: [PATCH v6 1/6] clk: Add of_clk_get_by_name_optional() function

[Phil Edworthy]

Hi Uwe,

On 16 November 2018 16:11 Uwe Kleine-König wrote:
> On Fri, Nov 16, 2018 at 05:01:28PM +0100, Uwe Kleine-König wrote:
> > Other than that I think the patch is fine
> 
> Thinking again, I wonder why not just do:
> 
> static inline struct clk *clk_get_optional(struct device *dev, const char 
> *id) {
>   struct clk *c = clk_get(dev, id);
> 
>   if (c == ERR_PTR(-ENOENT))
>   return NULL;
>   else
>   return c;
> }

Unfortunately, underneath this __of_clk_get_by_name() returns -EINVAL
when looking for a named clock, and the "clock-names" OF property can't
be found or the name is not in that prop. This is because the index
returned by of_property_match_string() will be an error code and is then
currently always passed to __of_clk_get().

If, as you said, I split the patches into one that fixes the error code, and 
then
adds clk_get_optional() like above, it will make more sense.

btw, do we need to add of_clk_get_by_name_optional()? I only added it as a
counterpart to of_clk_get_by_name(), but it may not be needed.

Thanks
Phil

RE: [PATCH v3 1/2] dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding

[Phil Edworthy]

Hi Rob,

On 17 November 2018 14:33 Rob Herring wrote:
> On Tue, Nov 13, 2018 at 01:09:09PM +0000, Phil Edworthy wrote:
> > Add device binding documentation for the Renesas RZ/N1 GPIO interrupt
> > multiplexer.
> >
> > Signed-off-by: Phil Edworthy 
> > ---
> > v3:
> >  - Use 'interrupt-map' DT property correctly.
> > v2:
> >  - Use interrupt-map to allow the GPIO controller info to be specified
> >as part of the irq.
> >  - Don't show status in binding examples.
> >  - Don't show the soc/board split in binding doc.
> > ---
> >  .../interrupt-controller/renesas,rzn1-mux.txt | 73
> > +++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mu
> > x.txt
> 
> A few nits, otherwise:
> 
> Reviewed-by: Rob Herring 
Thanks for the review!

> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-
> > mux.txt
> > b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-
> > mux.txt
> > new file mode 100644
> > index ..6515880e25cc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,r
> > +++ zn1-mux.txt
> > @@ -0,0 +1,73 @@
> > +* Renesas RZ/N1 GPIO Interrupt Multiplexer
> > +
> > +On Renesas RZ/N1 devices, there are several GPIO Controllers each
> > +with a number of interrupt outputs. All of the interrupts from the
> > +GPIO Controllers are passed to the GPIO Interrupt Multiplexer, which
> > +selects a sub-set of the interrupts to pass onto the system interrupt
> controller.
> > +
> > +A single node in the device tree is used to describe the GPIO IRQ Muxer.
> > +
> > +Required properties:
> > +- compatible: SoC-specific compatible string "renesas,-
> gpioirqmux"
> > +  followed by "renesas,rzn1-gpioirqmux" as fallback. The SoC-specific
> > +compatible
> > +  strings must be one of:
> > +   "renesas,r9a06g032-gpioirqmux" for RZ/N1D
> > +   "renesas,r9a06g033-gpioirqmux" for RZ/N1S
> > +- reg: Base address and size of GPIO IRQ Muxer registers.
> > +- interrupts: List of output interrupts.
> > +- #interrupt-cells: Numder of cells in the input interrupt specifier, must 
> > be
> 1.
> > +- #address-cells: Must be 0.
> > +- interrupt-map-mask: must be 127.
> > +- interrupt-map: Standard property detailing the maps between input
> > +irqs and the
> > +  corresponding output irq. This consist of a list of:
> > +   
> > +  The input-irq-spec is from 0 to 95, corresponding to the outputs of
> > +the GPIO
> > +  Controllers.
> > +
> > +Example:
> > +
> > +   The following is an example for the RZ/N1D SoC.
> > +
> > +   gpioirqmux: gpioirqmux@51000480 {
> 
> interrupt-controller@...
Sure

> > +   compatible = "renesas,r9a06g032-gpioirqmux",
> > +   "renesas,rzn1-gpioirqmux";
> > +   reg = <0x51000480 0x20>;
> > +   interrupts =
> > +   ,
> > +   ;
> 
> This is a bit redundant as the same information is in interrupt-map, but I
> guess you need it to get the irq resources.
That's right.

> > +
> > +   #interrupt-cells = <1>;
> > +   #address-cells = <0>;
> > +   interrupt-map-mask = <127>;
> 
> Use hex for masks.
Ok.

> > +   interrupt-map =
> > +   /* gpio2a 24, pin 146: ETH Port 1 IRQ */
> > +   <88  GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>,
> > +   /* gpio2a 26, pin 148: TouchSCRN_IRQ */
> > +   <90  GIC_SPI 104 IRQ_TYPE_LEVEL_HIGH>;
> > +   };
> > +
> > +   gpio2: gpio@5000d000 {
> > +   compatible = "snps,dw-apb-gpio";
> > +   reg = <0x5000d000 0x80>;
> > +   #address-cells = <1>;
> > +   #size-cells = <0>;
> > +   clock-names = "bus";
> > +   clocks = < R9A06G032_HCLK_GPIO2>;
> > +
> > +   gpio2a: gpio-controller@0 {
> 
> gpio@0
Are you sure about this?
The bindings in Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
show an example where the sub-nodes for gpio banks are gpio-controller@.
This is also in Documentation/devicetree/bindings/gpio/gpio.txt.

Thanks
Phil

> > +   compatible = "snps,dw-apb-gpio-port";
> > +   bank-name = "gpio2a";
> > +   gpio-controller;
> > +   #gpio-cells = <2>;
> > +   snps,nr-gpios = <32>;
> > +   reg = <0>;
> > +
> > +   interrupt-controller;
> > +   interrupt-parent = <>;
> > +   interrupts =  < 64 65 66 67 68 69 70 71
> > +   72 73 74 75 76 77 78 79
> > +   80 81 82 83 84 85 86 87
> > +   88 89 90 91 92 93 94 95 >;
> > +   #interrupt-cells = <2>;
> > +   };
> > +   };
> > --
> > 2.17.1
> >

RE: [PATCH v2] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

Hi Jacopo,

On 15 October 2018 16:12 jacopo mondi wrote:
> On Mon, Oct 15, 2018 at 04:01:47PM +0100, Phil Edworthy wrote:
> > This fixes the check for unused mdio bus setting and the following
> > static checker warning:
> >  drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
> >  warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max
> >= 0)'
> >
> > It also fixes the return var when calling of_get_child_count()
> >
> 
> Not really, since you skip the assignement if return value is <= 0:
> 
>   nfuncs = of_get_child_count(np);
>   if (nfuncs <= 0)
>   return 0;
> 
>   ipctl->nfunctions = nfuncs;
> 
> This seems more likely to be here to make 'rzn1_pmx_get_funcs_count()'
> happy, as it returns a signed integer, but since nfunctions is only assigned 
> if
> 'nfuncs' > 0, then the cast is safe there.
> 
> I would keep this unsigned, and rather return an error if
> 'of_get_child_count()' returns an error of any sort in 'probe_dt()', otherwise
> assign 'ipctl->nfunctions = nfuncs;' unconditionally and return immediately if
> nfuncs == 0. This makes sure nfunctions is initialized even if there are no
> functions registered.
Ok, I get what you're saying, though nfunctions will always be initialised due
to allocation with devm_kzalloc.

> Anyway, small issues, this just doesn't belong to this patch, but it's likely 
> not
> to cause any harm I guess.
True... should I leave this patch as is or respin again?

Thanks
Phil
 
> > Reported-by: Dan Carpenter 
> > Signed-off-by: Phil Edworthy 
> > ---
> > v2:
> >  - Don't use implicit type conversion.
> >  - Fix type of return var when calling of_get_child_count().
> > ---
> >  drivers/pinctrl/pinctrl-rzn1.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..4998463c54a0
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -112,13 +112,13 @@ struct rzn1_pinctrl {
> > struct rzn1_pinctrl_regs __iomem *lev2;
> > u32 lev1_protect_phys;
> > u32 lev2_protect_phys;
> > -   u32 mdio_func[2];
> > +   int mdio_func[2];
> >
> > struct rzn1_pin_group *groups;
> > unsigned int ngroups;
> >
> > struct rzn1_pmx_func *functions;
> > -   unsigned int nfunctions;
> > +   int nfunctions;
> >  };
> >
> >  #define RZN1_PINS_PROP "pinmux"
> > @@ -195,9 +195,9 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl
> > *ipctl, u8 lock, u8 value)  static void rzn1_pinctrl_mdio_select(struct
> rzn1_pinctrl *ipctl, int mdio,
> >  u32 func)
> >  {
> > -   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > +   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] !=
> > +(int)func)
> > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> > -   ipctl->mdio_func[mdio] = func;
> > +   ipctl->mdio_func[mdio] = (int)func;
> >
> > dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> >
> > @@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct
> platform_device *pdev,
> > struct device_node *np = pdev->dev.of_node;
> > struct device_node *child;
> > unsigned int maxgroups = 0;
> > -   unsigned int nfuncs = 0;
> > unsigned int i = 0;
> > +   int nfuncs = 0;
> > int ret;
> >
> > nfuncs = of_get_child_count(np);
> > --
> > 2.17.1
> >

[PATCH v2] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

This fixes the check for unused mdio bus setting and the following static
checker warning:
 drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
 warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)'

It also fixes the return var when calling of_get_child_count()

Reported-by: Dan Carpenter 
Signed-off-by: Phil Edworthy 
---
v2:
 - Don't use implicit type conversion.
 - Fix type of return var when calling of_get_child_count().
---
 drivers/pinctrl/pinctrl-rzn1.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
index ce05e3a00be2..4998463c54a0 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -112,13 +112,13 @@ struct rzn1_pinctrl {
struct rzn1_pinctrl_regs __iomem *lev2;
u32 lev1_protect_phys;
u32 lev2_protect_phys;
-   u32 mdio_func[2];
+   int mdio_func[2];
 
struct rzn1_pin_group *groups;
unsigned int ngroups;
 
struct rzn1_pmx_func *functions;
-   unsigned int nfunctions;
+   int nfunctions;
 };
 
 #define RZN1_PINS_PROP "pinmux"
@@ -195,9 +195,9 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 
lock, u8 value)
 static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio,
 u32 func)
 {
-   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
+   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != (int)func)
dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
-   ipctl->mdio_func[mdio] = func;
+   ipctl->mdio_func[mdio] = (int)func;
 
dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
 
@@ -810,8 +810,8 @@ static int rzn1_pinctrl_probe_dt(struct platform_device 
*pdev,
struct device_node *np = pdev->dev.of_node;
struct device_node *child;
unsigned int maxgroups = 0;
-   unsigned int nfuncs = 0;
unsigned int i = 0;
+   int nfuncs = 0;
int ret;
 
nfuncs = of_get_child_count(np);
-- 
2.17.1

RE: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

HI Jacopo,

On 15 October 2018 08:54 jacopo mondi wrote:
> On Fri, Oct 12, 2018 at 11:40:36AM +0100, Phil Edworthy wrote:
> > This fixes the check for unused mdio bus setting and the following
> > static checker warning:
> >  drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
> >  warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max
> >= 0)'
> >
> > Reported-by: Dan Carpenter 
> > Signed-off-by: Phil Edworthy 
> > ---
> >  drivers/pinctrl/pinctrl-rzn1.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-rzn1.c
> > b/drivers/pinctrl/pinctrl-rzn1.c index ce05e3a00be2..f688f3a29dfd
> > 100644
> > --- a/drivers/pinctrl/pinctrl-rzn1.c
> > +++ b/drivers/pinctrl/pinctrl-rzn1.c
> > @@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl
> > *ipctl, u8 lock, u8 value)  static void rzn1_pinctrl_mdio_select(struct
> rzn1_pinctrl *ipctl, int mdio,
> >  u32 func)
> >  {
> > -   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > +   if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func)
> > dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n",
> mdio);
> > ipctl->mdio_func[mdio] = func;
> >
> 
> MY understanding here is that the static checker complains because you are
> comparing a variable of unsigned type to negative values, and indeed you are
> treating mdio_func as signed in the driver code.
> 
> mdio_func is defined as:
> 
> struct rzn1_pinctrl {
> ...
>   u32 mdio_func[2];
> ...
> };
> 
> Then in probe function mdio_func gets intialized as:
> 
>   ipctl->mdio_func[0] = -1;
>   ipctl->mdio_func[1] = -1;
> 
> I think you could safely make mdio_func integers, or either define an
> INVALID value ( > 0), intialize them it that value and check against it for
> validity.

You are right, I shouldn't rely on the implicit typecast from -1 to uint.
I'll send an updated patch to fix this.

Thanks
Phil

[PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

This fixes the check for unused mdio bus setting and the following static
checker warning:
 drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
 warn: always true condition '(ipctl->mdio_func[mdio] >= 0) => (0-u32max >= 0)'

Reported-by: Dan Carpenter 
Signed-off-by: Phil Edworthy 
---
 drivers/pinctrl/pinctrl-rzn1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
index ce05e3a00be2..f688f3a29dfd 100644
--- a/drivers/pinctrl/pinctrl-rzn1.c
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -195,7 +195,7 @@ static void rzn1_hw_set_lock(struct rzn1_pinctrl *ipctl, u8 
lock, u8 value)
 static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio,
 u32 func)
 {
-   if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
+   if (ipctl->mdio_func[mdio] != -1 && ipctl->mdio_func[mdio] != func)
dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
ipctl->mdio_func[mdio] = func;
 
-- 
2.17.1

RE: [PATCH] pinctrl: renesas: fix platform_no_drv_owner.cocci warnings

[Phil Edworthy]

On 10 October 2018 17:03 kbuild test robot wrote:
> drivers/pinctrl/pinctrl-rzn1.c:935:3-8: No need to set .owner here. The core
> will do it.
> 
>  Remove .owner field if calls are used which set it automatically
> 
> Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
> 
> Fixes: 4e53b5004745 ("pinctrl: renesas: Renesas RZ/N1 pinctrl driver")
> CC: Phil Edworthy 
> Signed-off-by: kbuild test robot 
Reviewed-by: Phil Edworthy 


> ---
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-
> drivers.git sh-pfc-for-v4.20
> head:   ef26d96023a4c34b1bcc4294f570df2b63a1b952
> commit: 4e53b5004745ef26a37bca4933b2d3ea71313f2a [24/25] pinctrl:
> renesas: Renesas RZ/N1 pinctrl driver
> 
>  pinctrl-rzn1.c |1 -
>  1 file changed, 1 deletion(-)
> 
> --- a/drivers/pinctrl/pinctrl-rzn1.c
> +++ b/drivers/pinctrl/pinctrl-rzn1.c
> @@ -932,7 +932,6 @@ static struct platform_driver rzn1_pinct
>   .remove = rzn1_pinctrl_remove,
>   .driver = {
>   .name   = "rzn1-pinctrl",
> - .owner  = THIS_MODULE,
>   .of_match_table = rzn1_pinctrl_match,
>   },
>  };

[PATCH v6 3/3] ARM: dts: r9a06g032: Add pinctrl node

[Phil Edworthy]

This provides a pinctrl driver for the Renesas R9A06G032 SoC

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy 
---
v6:
 - No changes.

v5:
 - No changes.

v4:
 - No changes.

v3:
 - No changes.

v2:
 - Add "renesas,rzn1-pinctrl" compatible fallback string
 - Register size corrected.
---
 arch/arm/boot/dts/r9a06g032.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index eaf94976ed6d..2322268bc862 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -165,6 +165,14 @@
status = "disabled";
};
 
+   pinctrl: pin-controller@40067000 {
+   compatible = "renesas,r9a06g032-pinctrl", 
"renesas,rzn1-pinctrl";
+   reg = <0x40067000 0x1000>, <0x5100 0x480>;
+   clocks = < R9A06G032_HCLK_PINCONFIG>;
+   clock-names = "bus";
+   status = "okay";
+   };
+
gic: gic@44101000 {
compatible = "arm,cortex-a7-gic", "arm,gic-400";
interrupt-controller;
-- 
2.17.1

[PATCH v6 2/3] pinctrl: renesas: Renesas RZ/N1 pinctrl driver

[Phil Edworthy]

This provides a pinctrl driver for the Renesas RZ/N1 device family.

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy 
Reviewed-by: Jacopo Mondi 
---
v6:
 - Instead of combining the pin nr and func into a single element, use
   a pair of 8-bit elements.
 - Simplified how the MDIO function is calculated

v5:
 - Address Jacopo's comments
 - Address Geert's comments

v4:
 - Address Jacopo's comments
 - Implement pin_config_group_get()
 - Fix function to get pin configs, i.e. return -EINVAL when disabled.

v3:
 - Use standard DT props instead of proprietary ones.
 - Replace virtual pins used for MDIO muxing with extra funcs.
 - Use pinctrl_utils funcs to handle the maps.
 - Remove the dbg functions to keep things simple.

v2:
 - Change filename to generic rzn1, instead of device specific.
 - Changed Kconfig symbol and file name to generic rzn1 family.
 - Added "renesas,rzn1-pinctrl" compatible fallback string
 - Changes suggested by Jacopo Mondi. Mainly formatting, plus:
   - Removed global ptr
   - Removed unused code accessing parent of node.
   - Removed check for null OF np that can't happen.
   - Replaced overlapping enums with #defines
 - Renamed some variables and symbols to clarify their use
 - Fix error handling during probe
 - Move probe from postcore_initcall to subsys_initcall to ensure
   drivers that require clocks get them instead of having to defer
   probing.
---
 drivers/pinctrl/Kconfig|  10 +
 drivers/pinctrl/Makefile   |   1 +
 drivers/pinctrl/pinctrl-rzn1.c | 941 +
 3 files changed, 952 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rzn1.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 978b2ed4d014..4d8c00eac742 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -195,6 +195,16 @@ config PINCTRL_RZA1
help
  This selects pinctrl driver for Renesas RZ/A1 platforms.
 
+config PINCTRL_RZN1
+   bool "Renesas RZ/N1 pinctrl driver"
+   depends on OF
+   depends on ARCH_RZN1 || COMPILE_TEST
+   select GENERIC_PINCTRL_GROUPS
+   select GENERIC_PINMUX_FUNCTIONS
+   select GENERIC_PINCONF
+   help
+ This selects pinctrl driver for Renesas RZ/N1 devices.
+
 config PINCTRL_SINGLE
tristate "One-register-per-pin type device tree based pinctrl driver"
depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 8e127bd8610f..18a13c1e2c21 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_PINCTRL_PIC32)   += pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP) += pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_RZA1) += pinctrl-rza1.o
+obj-$(CONFIG_PINCTRL_RZN1) += pinctrl-rzn1.o
 obj-$(CONFIG_PINCTRL_SINGLE)   += pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF) += sirf/
 obj-$(CONFIG_PINCTRL_SX150X)   += pinctrl-sx150x.o
diff --git a/drivers/pinctrl/pinctrl-rzn1.c b/drivers/pinctrl/pinctrl-rzn1.c
new file mode 100644
index ..c1c9ee4c502f
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rzn1.c
@@ -0,0 +1,941 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2014-2018 Renesas Electronics Europe Limited
+ *
+ * Phil Edworthy 
+ * Based on a driver originally written by Michel Pollet at Renesas.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+#define RZN1_FUNC_L2_MAX   RZN1_FUNC_MAC_MTIP_SWITCH
+#define RZN1_FUNC_MDIO_MAX RZN1_FUNC_MDIO1_E1_SWITCH
+
+/* Field positions and masks in the pinmux registers */
+#define RZN1_L1_PIN_DRIVE_STRENGTH 10
+#define RZN1_L1_PIN_DRIVE_STRENGTH_4MA 0
+#define RZN1_L1_PIN_DRIVE_STRENGTH_6MA 1
+#define RZN1_L1_PIN_DRIVE_STRENGTH_8MA 2
+#define RZN1_L1_PIN_DRIVE_STRENGTH_12MA3
+#define RZN1_L1_PIN_PULL   8
+#define RZN1_L1_PIN_PULL_NONE  0
+#define RZN1_L1_PIN_PULL_UP1
+#define RZN1_L1_PIN_PULL_DOWN  3
+#define RZN1_L1_FUNCTION   0
+#define RZN1_L1_FUNC_MASK  0xf
+#define RZN1_L1_FUNCTION_L20xf
+
+/*
+ * The hardware manual describes two levels of multiplexing, but it's more
+ * logical to think of the hardware as three levels, with level 3 consisting of
+ * the multiplexing for Ethernet MDIO signals.
+ *
+ * Level 1 functions go from 0 to 9, with level 1 function '15' (0xf) 
specifying
+ * that level 2 functions are used instead. Level 2 has a lot more options,
+ * going from 0 to 61. Level 3 allows selection of MDIO functions which can be
+ * floating, or one of seven internal peripherals. Unfortunately, there are two
+ * level 2 functions that can select MDIO, and two MDIO channels so we have 
four
+

[PATCH v6 1/3] dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation

[Phil Edworthy]

The Renesas RZ/N1 device family PINCTRL node description.

Based on a patch originally written by Michel Pollet at Renesas.

Signed-off-by: Phil Edworthy 
Reviewed-by: Jacopo Mondi 
---
v6:
 - Instead of combining the pin nr and func into a single element, use
   a pair of 8-bit elements.

v5:
 - 'Optional generic properties' => 'Optional generic pinconf properties'

v4:
 - Add alternative way to use the pinmux prop.
 - Remove mention of gpios.

v3:
 - Use standard bindings
 - Change the way the functions are defined so it is easy to check
   against the hardware numbering.
 - Add functions for the MDIO source peripheral instead of using
   virtual pins.

v2:
 - Change filename to generic rzn1, instead of device specific.
 - Add "renesas,rzn1-pinctrl" compatible fallback string.
 - Example register size corrected.
 - Typos fixed.
 - Changes suggested by Jacopo Mondi.
 - rzn1-pinctrl.h squashed into this as requested by Rob Herring.
---
 .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 155 ++
 include/dt-bindings/pinctrl/rzn1-pinctrl.h| 135 +++
 2 files changed, 290 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
 create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt 
b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
new file mode 100644
index ..ca747b3b7455
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
@@ -0,0 +1,155 @@
+Renesas RZ/N1 SoC Pinctrl node description.
+
+Pin controller node
+---
+Required properties:
+- compatible: SoC-specific compatible string "renesas,-pinctrl"
+  followed by "renesas,rzn1-pinctrl" as fallback. The SoC-specific compatible
+  strings must be one of:
+   "renesas,r9a06g032-pinctrl" for RZ/N1D
+   "renesas,r9a06g033-pinctrl" for RZ/N1S
+- reg: Address base and length of the memory area where the pin controller
+  hardware is mapped to.
+- clocks: phandle for the clock, see the description of clock-names below.
+- clock-names: Contains the name of the clock:
+"bus", the bus clock, sometimes described as pclk, for register accesses.
+
+Example:
+   pinctrl: pin-controller@40067000 {
+   compatible = "renesas,r9a06g032-pinctrl", "renesas,rzn1-pinctrl";
+   reg = <0x40067000 0x1000>, <0x5100 0x480>;
+   clocks = < R9A06G032_HCLK_PINCONFIG>;
+   clock-names = "bus";
+   };
+
+Sub-nodes
+-
+
+The child nodes of the pin controller node describe a pin multiplexing
+function.
+
+- Pin multiplexing sub-nodes:
+  A pin multiplexing sub-node describes how to configure a set of
+  (or a single) pin in some desired alternate function mode.
+  A single sub-node may define several pin configurations.
+  Please refer to pinctrl-bindings.txt to get to know more on generic
+  pin properties usage.
+
+  The allowed generic formats for a pin multiplexing sub-node are the
+  following ones:
+
+  node-1 {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+
+  node-2 {
+  sub-node-1 {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+
+  sub-node-2 {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+
+  ...
+
+  sub-node-n {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+  };
+
+  node-3 {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+
+  sub-node-1 {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+
+  ...
+
+  sub-node-n {
+  pinmux = /bits/ 8 , , ... ;
+  GENERIC_PINCONFIG;
+  };
+  };
+
+  Use the latter two formats when pins part of the same logical group need to
+  have different generic pin configuration flags applied. Note that the generic
+  pinconfig in node-3 does not apply to the sub-nodes.
+
+  Client sub-nodes shall refer to pin multiplexing sub-nodes using the phandle
+  of the most external one.
+
+  Eg.
+
+  client-1 {
+  ...
+  pinctrl-0 = <>;
+  ...
+  };
+
+  client-2 {
+  ...
+  pinctrl-0 = <>;
+  ...
+  };
+
+  Required properties:
+- pinmux:
+  8-bit integer array consisting of PIN_NR pin number and MUX_FUNC pairs.
+  When a pin has to be configured in alternate function mode, use this
+  property to identify the pin by its global index, and provide its
+  alternate function configuration number.
+  When multiple pins are required to be configured as part of the same
+  alternate function they shall be specified as members of the same
+  argument list of a single "pinmux" property.
+  PIN_NR directly corresponds to the pl_gpio pin number and MUX_FUNC is
+  o

[PATCH v6 0/3] Renesas R9A06G032 PINCTRL Driver

[Phil Edworthy]

This implements the pinctrl driver for the RZ/N1 family of devices, including
the R9A06G032 (RZ/N1D) device.

This series was originally written by Michel Pollet whilst at Renesas, and I
have taken over this work.

Main changes:
v6:
 - Instead of combining the pin nr and func into a single element, use
   a pair of 8-bit elements.
 - Simplified how the MDIO function is calculated

v5:
 - Address Jacopo's further comments
 - Address Geert's comments

v4:
 - Address Jacopo's comments
 - Add alternative way to use the pinmux prop.
 - Remove mention of gpios.
 - Implement pin_config_group_get()
 - Fix function to get pin configs, i.e. return -EINVAL when disabled.

v3:
 - Use standard DT props instead of proprietary ones.
 - Replace virtual pins used for MDIO muxing with extra funcs.
 - Use pinctrl_utils funcs to handle the maps.
 - Remove the dbg functions to keep things simple.
 - Change the way the functions are defined so it is easy to check
   against the hardware numbering.

v2:
 - Change to generic rzn1 family driver, instead of device specific.
 - Review comments fixed.
 - Fix error handling during probe

Phil Edworthy (3):
  dt-bindings: pinctrl: renesas,rzn1-pinctrl: documentation
  pinctrl: renesas: Renesas RZ/N1 pinctrl driver
  ARM: dts: r9a06g032: Add pinctrl node

 .../bindings/pinctrl/renesas,rzn1-pinctrl.txt | 155 +++
 arch/arm/boot/dts/r9a06g032.dtsi  |   8 +
 drivers/pinctrl/Kconfig   |  10 +
 drivers/pinctrl/Makefile  |   1 +
 drivers/pinctrl/pinctrl-rzn1.c| 941 ++
 include/dt-bindings/pinctrl/rzn1-pinctrl.h| 135 +++
 6 files changed, 1250 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/pinctrl/renesas,rzn1-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rzn1.c
 create mode 100644 include/dt-bindings/pinctrl/rzn1-pinctrl.h

-- 
2.17.1

[PATCH v2] ARM: dts: r9a06g032: Correct UART and add all other UARTs

[Phil Edworthy]

- UART0 was missing the bus clock ("apb_pclk").
- Now that the relevant rzn1 bindings have been added, replace the Synopsys
  compat string with the rzn1 strings.
- Add all the other UARTs.

Signed-off-by: Phil Edworthy 
---
v2:
 - Keep the "snps,dw-apb-uart" fallback for uarts 0..2 as these are unmodified
   peripherals.
---
 arch/arm/boot/dts/r9a06g032.dtsi | 83 ++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 3e45375b79aa..eaf94976ed6d 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -78,13 +78,90 @@
};
 
uart0: serial@4006 {
-   compatible = "snps,dw-apb-uart";
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart", "snps,dw-apb-uart";
reg = <0x4006 0x400>;
interrupts = ;
reg-shift = <2>;
reg-io-width = <4>;
-   clocks = < R9A06G032_CLK_UART0>;
-   clock-names = "baudclk";
+   clocks = < R9A06G032_CLK_UART0>, < 
R9A06G032_HCLK_UART0>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart1: serial@40061000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart", "snps,dw-apb-uart";
+   reg = <0x40061000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART1>, < 
R9A06G032_HCLK_UART1>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart2: serial@40062000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart", "snps,dw-apb-uart";
+   reg = <0x40062000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART2>, < 
R9A06G032_HCLK_UART2>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart3: serial@5000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x5000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART3>, < 
R9A06G032_HCLK_UART3>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart4: serial@50001000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50001000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART4>, < 
R9A06G032_HCLK_UART4>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart5: serial@50002000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50002000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART5>, < 
R9A06G032_HCLK_UART5>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart6: serial@50003000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50003000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART6>, < 
R9A06G032_HCLK_UART6>;
+  

RE: [PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs

[Phil Edworthy]

Hi Geert,

On 06 September 2018 15:38 Geert Uytterhoeven wrote:
> On Thu, Aug 30, 2018 at 11:52 AM Phil Edworthy wrote:
> > - UART0 was missing the bus clock ("apb_pclk").
> > - Now that the relevant rzn1 bindings have been added, replace the
> Synopsys
> >   compat string with the rzn1 strings.
> > - Add all the other UARTs.
> >
> > Signed-off-by: Phil Edworthy 
> 
> Thanks for your patch!
And thanks for your review!
 
> Reviewed-by: Geert Uytterhoeven 
> 
> But a few notes/questions below.
> 
> > --- a/arch/arm/boot/dts/r9a06g032.dtsi
> > +++ b/arch/arm/boot/dts/r9a06g032.dtsi
> > @@ -78,13 +78,90 @@
> > };
> >
> > uart0: serial@4006 {
> > -   compatible = "snps,dw-apb-uart";
> > +   compatible = "renesas,r9a06g032-uart", 
> > "renesas,rzn1-uart";
> 
> Apparently not all 8 UARTs are identical: the first 3 don't have DMA, the
> last 5 do, and they have more registers.
> 
> Are you sure no different compatible values are needed to distinguish
> between them?
> Can you handle them purely based on the presence or absence of
> "dmas" properties (which are not yet present)?
Yes, I am pretty sure... the extra registers for the last 5 uarts are only dma
related and we have been using the standard Synopsys driver on the other
3 uarts without issue for some time.

> According to commit 72b0505f0830df95 ("dt: serial: Add Renesas RZ/N1
> binding documentation"), the Synopsis compatible string would work if you
> are not using DMA, so perhaps it should be added for the ports that cannot
> do DMA?
Makes sense, I will make them:
compatible = "renesas,r9a06g032-uart", "renesas,rzn1-uart", "snps,dw-apb-uart";

> > reg = <0x4006 0x400>;
> > interrupts = ;
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > -   clocks = < R9A06G032_CLK_UART0>;
> > -   clock-names = "baudclk";
> > +   clocks = < R9A06G032_CLK_UART0>, <
> R9A06G032_HCLK_UART0>;
> 
> It's a pity the clock names don't match the datasheet, but the output from
> the internal Renesas tools. So I have to trust you to not list them
> in the wrong order.
True... Note that all bus clocks required to access the peripheral's registers
are 'HCLK', other clocks such as baud clks are 'CLK'.

> > +   clock-names = "baudclk", "apb_pclk";
> > +   status = "disabled";
> > +   };

Thanks
Phil

[PATCH] ARM: dts: r9a06g032: Correct UART and add all other UARTs

[Phil Edworthy]

- UART0 was missing the bus clock ("apb_pclk").
- Now that the relevant rzn1 bindings have been added, replace the Synopsys
  compat string with the rzn1 strings.
- Add all the other UARTs.

Signed-off-by: Phil Edworthy 
---
 arch/arm/boot/dts/r9a06g032.dtsi | 83 ++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi
index 3e45375..1bc1f36 100644
--- a/arch/arm/boot/dts/r9a06g032.dtsi
+++ b/arch/arm/boot/dts/r9a06g032.dtsi
@@ -78,13 +78,90 @@
};
 
uart0: serial@4006 {
-   compatible = "snps,dw-apb-uart";
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
reg = <0x4006 0x400>;
interrupts = ;
reg-shift = <2>;
reg-io-width = <4>;
-   clocks = < R9A06G032_CLK_UART0>;
-   clock-names = "baudclk";
+   clocks = < R9A06G032_CLK_UART0>, < 
R9A06G032_HCLK_UART0>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart1: serial@40061000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x40061000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART1>, < 
R9A06G032_HCLK_UART1>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart2: serial@40062000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x40062000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART2>, < 
R9A06G032_HCLK_UART2>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart3: serial@5000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x5000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART3>, < 
R9A06G032_HCLK_UART3>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart4: serial@50001000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50001000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART4>, < 
R9A06G032_HCLK_UART4>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart5: serial@50002000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50002000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART5>, < 
R9A06G032_HCLK_UART5>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart6: serial@50003000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50003000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART6>, < 
R9A06G032_HCLK_UART6>;
+   clock-names = "baudclk", "apb_pclk";
+   status = "disabled";
+   };
+
+   uart7: serial@50004000 {
+   compatible = "renesas,r9a06g032-uart", 
"renesas,rzn1-uart";
+   reg = <0x50004000 0x400>;
+   interrupts = ;
+   reg-shift = <2>;
+   reg-io-width = <4>;
+   clocks = < R9A06G032_CLK_UART7>, < 
R9A06G032_HCLK_UART7>;
+   clock-names = "baudclk", "apb_pclk";
status = "disabled";
};
 
-- 
2.7.4

RE: [PATCH] ARM: dts: r9a06g032: Use r9a06g032-sysctrl binding definitions

[Phil Edworthy]

Hi Geert,

On 28 August 2018 16:13, Geert Uytterhoeven wrote:
> Replace the hardcoded clock indices by R9A06G032_CLK_* symbols.
> 
> Signed-off-by: Geert Uytterhoeven 

Reviewed-by: Phil Edworthy 

Thanks!
Phil

RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling

[Phil Edworthy]

Hi Lorenzo,

On 21 August 2018 16:32, Lorenzo Pieralisi wrote:
> On Tue, Aug 21, 2018 at 08:58:38AM +0000, Phil Edworthy wrote:
> > On 20 August 2018 15:48 Lorenzo Pieralisi wrote:
> > > On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote:
> > >
> > > [...]
> > >
> > > > However, both before and after this patch, the RP does not
> > > > transition
> > > > L1 when the endpoints change to L1.
> > > > This patch only transitions the RP to L1 during accessing a card's
> > > > config registers, if the RP is not in L1 link state and has
> > > > received
> > > > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will
> > > > handle the transition out of L1.
> > > >
> > > > The relevant part of the rcar manual says: "After a recovery to
> > > > L0, if the device is in the Non-D0 state and PM_Enter_L1 DLLP is
> > > > transmitted from the downstream device, software should confirm
> > > > that hardware is in the L0 state (PMSR.PMSTATE = L0) and initiate
> > > > the L1 transition sequence again (write 1 to PMCTLR.L1IATN). In
> > > > this case, the sequence
> > > > is: L0 ??? L1 ??? L0 recovery ??? L1 again."
> > >
> > > Can you map these FSM steps to this patch code please ? I would like
> > > to understand what Link state maps to which command written and
> when.
> > I don't think I can because we are not initially entering L1. Looking
> > at this again, I think this section of the manual only helps in
> > indicating how to detect we should have gone into L1 and how to poke
> > the HW to initiate the transition to L1.
> >
> > On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state.
> 
> I am still struggling to understand what "EP enters L1 state" means. A link in
> L1 means both ends of the link are in electrical idle, it is a two-way
> handshake, see PCI express specifications 5.3.2.1 "Entry into the L1 state".
Sorry, I'm no PCIe expert and the rcar HW documentation doesn't provide
enough detail. I guess (that's all I can do with this) the following:
a) the EP sends the PM_Enter_L1 DLLP,
b) the RP sends a PM_Request_Ack DLLP.
c) The EP physical layer is then inactive.
However, the rcar RP doesn't complete L1 transition, so the RP physical layer
is still active. Hence the EP thinks it is in L1, but the RP is not.


> > The rcar RP cannot enter L1 by HW alone, so is still in L0.
> 
> See above.
> 
> > The only way out of this from the PCIe spec FSM is for both EP and RP
> > to enter the Recovery state.
> > The patch simply detects that we should have gone into L1, and so
> > initiates that state change, and the HW will then handle the
> > transition from L1 to Recovery and then on to L0.
> 
> That I can understand, I reckon it is to "reset" the RP link state machine to 
> a
> "sane" state.


Bjorn's comment added back:
> I think there's still a potential issue if the endpoint goes to a
> non-D0 state, the link is stuck in this transitional state (endpoint 
> thinks it's L1, RP thinks it's L0), and the *endpoint* wants to exit 
> L1, e.g., so it can send a PME message for a wakeup.  I don't know 
> what happens then.

> > > > I don't think the potential issue that Bjorn talked about can
> > > > happen because the RP does go into L1. I could be wrong though...
> > >
> > > I do not understand this paragraph, mind elaborating on it ?
> > As rcar RP only supports D0 and D3hot/cold, (the manual says it
> > supports D3cold, but I cannot see how if it doesn't support L2 or L3
> > states), if you force the link to D3, we can only be in L1 state.
> 
> D3 is a device state, not a link state. I still do not understand this 
> statement.
> 
> The link between RP and EP can enter L1 when all functions in the EP are in a
> device state != D0 but, as I mentioned above, it is still unclear what happens
> in this platform since I do not get what state in the PCI spec 5.3.2.1 state
> machine the RP Link state machine is in.
> 
> If we programme the device into any D-state and the device wants to send a
> PME message _before_ we reset the RP state machine with the procedure
> described in this thread, what happens ? Or, more explicitly, what are in
> _HW_ the states of upstream and downstream link state machines when the
> EP is put in, say, D1 ?
rcar only supports D0 and D3, and L0/L0s/L1 so it's a bit simpler (I assume
devices can only be put into D states that are supported by the RP). 
If I read the PCIe spec 5.3.2 correctly, for rcar, if the device is put into D3,
the i

RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling

[Phil Edworthy]

Hi Lorenzo,

On 20 August 2018 15:48 Lorenzo Pieralisi wrote:
> On Mon, Aug 20, 2018 at 01:44:48PM +0000, Phil Edworthy wrote:
> 
> [...]
> 
> > However, both before and after this patch, the RP does not transition
> > L1 when the endpoints change to L1.
> > This patch only transitions the RP to L1 during accessing a card's
> > config registers, if the RP is not in L1 link state and has received
> > PM_ENTER_L1 DLLP (e.g. resume). After this, the hardware will handle
> > the transition out of L1.
> >
> > The relevant part of the rcar manual says: "After a recovery to L0, if
> > the device is in the Non-D0 state and PM_Enter_L1 DLLP is transmitted
> > from the downstream device, software should confirm that hardware is
> > in the L0 state (PMSR.PMSTATE = L0) and initiate the L1 transition
> > sequence again (write 1 to PMCTLR.L1IATN). In this case, the sequence
> > is: L0 → L1 → L0 recovery → L1 again."
> 
> Can you map these FSM steps to this patch code please ? I would like to
> understand what Link state maps to which command written and when.
I don’t think I can because we are not initially entering L1. Looking at this
again, I think this section of the manual only helps in indicating how to 
detect we should have gone into L1 and how to poke the HW to initiate the
transition to L1.

On system suspend, the EP sends PM_Enter_L1 DLLP and enters L1 state.
The rcar RP cannot enter L1 by HW alone, so is still in L0. The only way out
of this from the PCIe spec FSM is for both EP and RP to enter the Recovery
state.
The patch simply detects that we should have gone into L1, and so initiates
that state change, and the HW will then handle the transition from L1 to
Recovery and then on to L0.


> > I don’t think the potential issue that Bjorn talked about can happen
> > because the RP does go into L1. I could be wrong though...
> 
> I do not understand this paragraph, mind elaborating on it ?
As rcar RP only supports D0 and D3hot/cold, (the manual says it supports
D3cold, but I cannot see how if it doesn’t support L2 or L3 states), if you
force the link to D3, we can only be in L1 state.


> > The driver should also have a runtime-PM hook to transition to L1 on
> > suspend in order to save power. However, that is somewhat separate to
> > the problem the patch fixes.
> 
> Yes that's a separate patch.
> 
> Thanks for chiming in, let's try to get to the bottom of this thread.
> 
> Lorenzo

Thanks
Phil

RE: [PATCH V2 4/5] PCI: rcar: Support runtime PM, link state L1 handling

[Phil Edworthy]

Hello,

Sorry for the delay.

On 08 August 2018 14:30, Marek Vasut wrote:
> On 07/25/2018 11:08 PM, Marek Vasut wrote:
> > On 06/13/2018 07:25 PM, Bjorn Helgaas wrote:
> >> On Wed, Jun 13, 2018 at 04:52:52PM +0100, Lorenzo Pieralisi wrote:
> >>> On Wed, Jun 13, 2018 at 08:53:08AM -0500, Bjorn Helgaas wrote:
> >>>> On Wed, Jun 13, 2018 at 01:54:51AM +0200, Marek Vasut wrote:
> >>>>> On 06/11/2018 03:59 PM, Bjorn Helgaas wrote:
> >>>>>> On Sun, Jun 10, 2018 at 03:57:10PM +0200, Marek Vasut wrote:
> >>>>>>> On 11/17/2017 06:49 PM, Lorenzo Pieralisi wrote:
> >>>>>>>> On Fri, Nov 10, 2017 at 10:58:42PM +0100, Marek Vasut wrote:
> >>>>>>>>> From: Phil Edworthy 
> >>>>>>>>>
> >>>>>>>>> Most PCIe host controllers support L0s and L1 power states via
> ASPM.
> >>>>>>>>> The R-Car hardware only supports L0s, so when the system
> >>>>>>>>> suspends and resumes we have to manually handle L1.
> >>>>>>>>> When the system suspends, cards can put themselves into L1
> and
> >>>>>>>>> send a
> >>>>>>>>
> >>>>>>>> I assumed L1 entry has to be negotiated depending upon the PCIe
> >>>>>>>> hierarchy capabilities, I would appreciate if you can explain
> >>>>>>>> to me what's the root cause of the issue please.
> >>>>>>>
> >>>>>>> You should probably ignore the suspend/resume part altogether.
> >>>>>>> The issue here is that the cards can enter L1 state, while the
> >>>>>>> controller won't do that automatically, it can only detect that the
> link went into L1 state.
> >>>>>>> If that happens,the driver must manually put the controller to L1
> state.
> >>>>>>> The controller can transition out of L1 state automatically though.
> >>>>>>
> >>>>>> From earlier discussion I thought the R-Car root port did not
> >>>>>> advertise L1 support.
> >>>>>
> >>>>> Which discussion ? This one or somewhere else ?
> >>>>
> >>>>
> https://lkml.kernel.org/r/HK2PR0601MB1393D917D343E6363484CA68F5CB0
> @
> >>>> HK2PR0601MB1393.apcprd06.prod.outlook.com
> >>>>
> >>>> Re-reading that, I think I see my misunderstanding.  I was only
> >>>> considering L1 in the ASPM context.  I didn't realize the L1
> >>>> implications of devices being in states other than D0.
> >>>>
> >>>> Obviously L1 support for ASPM is optional and advertised via Link
> >>>> Capabilities.  But per PCIe r4.0, sec 5.2, L1 support is required
> >>>> for PCI-PM compatible power management, and is entered "whenever
> >>>> all Functions ... are programmed to a D-state other than D0."
> >>>>
> >>>> So I guess this means *every* device is supposed to support L1 when
> >>>> it is in a non-D0 power state.  I think *this* is the case you're
> >>>> solving.
> >>>>
> >>>> A little more of this detail, e.g., that this issue has nothing to
> >>>> do with ASPM, it's probably an R-Car erratum that the RC can't
> >>>> transition from L1 to L0, etc., in the changelog would really help
> >>>> clear things up for me.
> >>>
> >>> I think that the issue is related to the L0->L1 transition upon
> >>> system suspend (ie the kernel must force the controller into L1 when
> >>> all devices are in a sleep state) and for this specific reason I
> >>> still think that checking for a PM_Enter_L1 DLLP reception and doing
> >>> the L0->L1 transition within a config access is wrong and prone to
> >>> error (what's the rationale behind that ?), this ought to be done
> >>> using PM methods in the host controller driver.
> >>
> >> But doesn't the problem happen whenever the link goes to L1, for any
> >> reason?  E.g., runtime power management might put an endpoint in D3
> >> even if we're not doing a whole system suspend.  A user could even
> >> force the endpoint to D3 by writing to PCI_PM_CTRL with "setpci".  If
> >> that's the case, I don't think the host controller PM methods will be
> >> enough to work around the issue.
>

[PATCH v2 2/2] serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

[Phil Edworthy]

The Renesas RZ/N1 UART is based on the Synopsys DW UART, but has additional
registers for DMA. This patch does not address the changes required for DMA
support, it simply adds the compatible string.

Signed-off-by: Phil Edworthy 
---
v2:
 - Change "renesas,-" to
   "renesas,-"
---
 drivers/tty/serial/8250/8250_dw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c 
b/drivers/tty/serial/8250/8250_dw.c
index aff04f1..7a7c742 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -693,6 +693,7 @@ static const struct of_device_id dw8250_of_match[] = {
{ .compatible = "snps,dw-apb-uart" },
{ .compatible = "cavium,octeon-3860-uart" },
{ .compatible = "marvell,armada-38x-uart" },
+   { .compatible = "renesas,rzn1-uart" },
{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
-- 
2.7.4

[PATCH v2 1/2] dt: serial: Add Renesas RZ/N1 binding documentation

[Phil Edworthy]

The RZ/N1 UART is a modified Synopsys DesignWare UART.
The modifications only relate to DMA so you could actually use the
controller with the Synopsys compatible string if you are not using
DMA, but you should not do so.

Signed-off-by: Phil Edworthy 
---
v2:
 - Change "renesas,-" to
   "renesas,-"
---
 Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt | 10 ++
 1 file changed, 10 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt

diff --git a/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt 
b/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt
new file mode 100644
index 000..8b9e0d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt
@@ -0,0 +1,10 @@
+Renesas RZ/N1 UART
+
+This controller is based on the Synopsys DesignWare ABP UART and inherits all
+properties defined in snps-dw-apb-uart.txt except for the compatible property.
+
+Required properties:
+- compatible : The device specific string followed by the generic RZ/N1 string.
+   Therefore it must be one of:
+   "renesas,r9a06g032-uart", "renesas,rzn1-uart"
+   "renesas,r9a06g033-uart", "renesas,rzn1-uart"
-- 
2.7.4

[PATCH v2 0/2] dt: serial: Add Renesas RZ/N1 binding documentation

[Phil Edworthy]

Just a new compatible string for the Synopsys UART to allow us to add DMA at
some point in the future.

v2:
 - Change "renesas,-" to
   "renesas,-"

Phil Edworthy (2):
  dt: serial: Add Renesas RZ/N1 binding documentation
  serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

 Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt | 10 ++
 drivers/tty/serial/8250/8250_dw.c  |  1 +
 2 files changed, 11 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt

-- 
2.7.4

RE: [PATCH 1/2] dt: serial: Add Renesas RZ/N1 binding documentation

[Phil Edworthy]

Hi Geert,

On 11 July 2018 13:39, Geert Uytterhoeven wrote:
> On Wed, Jul 11, 2018 at 2:30 PM Phil Edworthy wrote:
> > The RZ/N1 UART is a modified Synopsys DesignWare UART.
> > The modifications only relate to DMA so you could actually use the
> > controller with the Synopsys compatible string if you are not using
> > DMA, but you should not do so.
> >
> > Signed-off-by: Phil Edworthy 
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt
> > @@ -0,0 +1,10 @@
> > +Renesas RZ/N1 UART
> > +
> > +This controller is based on the Synopsys DesignWare ABP UART and
> > +inherits all properties defined in snps-dw-apb-uart.txt except for the
> compatible property.
> > +
> > +Required properties:
> > +- compatible : The device specific string followed by the generic RZ/N1
> string.
> > +   Therefore it must be one of:
> > +   "renesas,uart-r9a06g032", "renesas,uart-rzn1"
> 
> "renesas,r9a06g032-uart", "renesas,rzn1-uart"
> 
> > +   "renesas,uart-r9a06g033", "renesas,uart-rzn1"
> 
> "renesas,r9a06g033-uart", "renesas,rzn1-uart"
I followed the renesas convention of renesas,peripheral-device that is used for
most R-Car drivers, but happy to change.

> I assume you plan to describe the DMA-related properties later?
Yes, it will take a while to sort out.

Thanks
Phil

RE: [PATCH 2/2] serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

[Phil Edworthy]

Hi Geert,

On 11 July 2018 16:02, Phil Edworthy wrote:
> On 11 July 2018 13:42, Geert Uytterhoeven wrote:
> > On Wed, Jul 11, 2018 at 2:30 PM Phil Edworthy wrote:
> > > The Renesas RZ/N1 UART is based on the Synopsys DW UART, but has
> > > additional registers for DMA. This patch does not address the
> > > changes required for DMA support, it simply adds the compatible string.
> > >
> > > Signed-off-by: Phil Edworthy 
> >
> > Thanks for your patch!
> >
[...]
> > > --- a/drivers/tty/serial/8250/8250_dw.c
> > > +++ b/drivers/tty/serial/8250/8250_dw.c
> > > @@ -693,6 +693,7 @@ static const struct of_device_id
> > > dw8250_of_match[]
> > = {
> > > { .compatible = "snps,dw-apb-uart" },
> > > { .compatible = "cavium,octeon-3860-uart" },
> > > { .compatible = "marvell,armada-38x-uart" },
> > > +   { .compatible = "renesas,uart-rzn1" },
> >
> > renesas,rzn1-uart
I missed this comment. I followed the renesas convention of 
renesas,peripheral-device
that is used for most R-Car drivers, but happy to change. 

Thanks
Phil

RE: [PATCH 2/2] serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

[Phil Edworthy]

Hi Geert,

On 11 July 2018 13:42, Geert Uytterhoeven wrote:
> On Wed, Jul 11, 2018 at 2:30 PM Phil Edworthy wrote:
> > The Renesas RZ/N1 UART is based on the Synopsys DW UART, but has
> > additional registers for DMA. This patch does not address the changes
> > required for DMA support, it simply adds the compatible string.
> >
> > Signed-off-by: Phil Edworthy 
> 
> Thanks for your patch!
> 
> What happens if someone would boot a kernel that has only this patch
> applied and a DTB that already has the to-be-supported dmas properties?
The driver only sets up dma if the fifo depth is >0, and this is read from
CPR register. This is an optional (as in RTL configuration) register for the
Synopsys UART block. On rzn1 devices, this register returns 0, so the
driver will not set up dma => so the uart still works.

Additionally, the uart normally used for the console (used because the 
BootROM uses it) does not have any dma capabilities.

Thanks
Phil

> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -693,6 +693,7 @@ static const struct of_device_id dw8250_of_match[]
> = {
> > { .compatible = "snps,dw-apb-uart" },
> > { .compatible = "cavium,octeon-3860-uart" },
> > { .compatible = "marvell,armada-38x-uart" },
> > +   { .compatible = "renesas,uart-rzn1" },
> 
> renesas,rzn1-uart
> 
> > { /* Sentinel */ }
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

[PATCH 2/2] serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

[Phil Edworthy]

The Renesas RZ/N1 UART is based on the Synopsys DW UART, but has additional
registers for DMA. This patch does not address the changes required for DMA
support, it simply adds the compatible string.

Signed-off-by: Phil Edworthy 
---
 drivers/tty/serial/8250/8250_dw.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c 
b/drivers/tty/serial/8250/8250_dw.c
index aff04f1..c92f5ad 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -693,6 +693,7 @@ static const struct of_device_id dw8250_of_match[] = {
{ .compatible = "snps,dw-apb-uart" },
{ .compatible = "cavium,octeon-3860-uart" },
{ .compatible = "marvell,armada-38x-uart" },
+   { .compatible = "renesas,uart-rzn1" },
{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, dw8250_of_match);
-- 
2.7.4

[PATCH 1/2] dt: serial: Add Renesas RZ/N1 binding documentation

[Phil Edworthy]

The RZ/N1 UART is a modified Synopsys DesignWare UART.
The modifications only relate to DMA so you could actually use the
controller with the Synopsys compatible string if you are not using
DMA, but you should not do so.

Signed-off-by: Phil Edworthy 
---
 Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt | 10 ++
 1 file changed, 10 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt

diff --git a/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt 
b/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt
new file mode 100644
index 000..4d40587
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt
@@ -0,0 +1,10 @@
+Renesas RZ/N1 UART
+
+This controller is based on the Synopsys DesignWare ABP UART and inherits all
+properties defined in snps-dw-apb-uart.txt except for the compatible property.
+
+Required properties:
+- compatible : The device specific string followed by the generic RZ/N1 string.
+   Therefore it must be one of:
+   "renesas,uart-r9a06g032", "renesas,uart-rzn1"
+   "renesas,uart-r9a06g033", "renesas,uart-rzn1"
-- 
2.7.4

[PATCH 0/2] dt: serial: Add Renesas RZ/N1 binding documentation

[Phil Edworthy]

Just a new compatible string for the Synopsys UART to allow us to add DMA at
some point in the future.

Phil Edworthy (2):
  dt: serial: Add Renesas RZ/N1 binding documentation
  serial: 8250_dw: Add compatible string for Renesas RZ/N1 UART

 Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt | 10 ++
 drivers/tty/serial/8250/8250_dw.c  |  1 +
 2 files changed, 11 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/serial/renesas,rzn1-uart.txt

-- 
2.7.4

[PATCH] gpio: dwapb: Fix rework support for 1 interrupt per port A GPIO

[Phil Edworthy]

Commit da069d5d2b814d9887989dcdb29fb0202eac8b38 ("gpio: dwapb: Rework
support for 1 interrupt per port A GPIO"), was an incremental patch that
was supposed to provide the delta between v5 and v6 patch set for
adding support for 1 interupt per port A GPIO. v5 was applied, then some
other feedback came afterwards.

However, in my haste I managed to drop the changes made to dwapb_port_property
struct. This patch includes those missing changes.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 include/linux/platform_data/gpio-dwapb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/platform_data/gpio-dwapb.h 
b/include/linux/platform_data/gpio-dwapb.h
index 5a52d69..419cfac 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -19,7 +19,7 @@ struct dwapb_port_property {
unsigned intidx;
unsigned intngpio;
unsigned intgpio_base;
-   unsigned intirq[32];
+   int irq[32];
boolhas_irq;
boolirq_shared;
 };
-- 
2.7.4

RE: [PATCH] gpio: dwapb: Rework support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Simon,

On 23 May 2018 10:12 Simon Horman wrote:
> On Wed, May 23, 2018 at 09:52:44AM +0100, Phil Edworthy wrote:
> > Treat DT and ACPI the same as much as possible. Note that we can't use
> > platform_get_irq() to get the DT interrupts as they are in the port
> > sub-node and hence do not have an associated platform device.
> >
> > This also fixes a problem introduced with error checking when calling
> > platform_get_irq().
> 
> What is the problem? In general I think fixes should be in separate patches.
The ACPI code would ignore errors returned by platform_get_irq(), instead
treating the error as an interrupt number.

Andy Shevchenko provided some late feedback to the v5 patch, but v5 was 
already applied by Linus W. This patch just has the incremental changes that
were made in v6 of the patch.

BR
Phil

> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> >  drivers/gpio/gpio-dwapb.c | 53
> > ---
> >  1 file changed, 22 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> > index 7dcd06b..15b4154 100644
> > --- a/drivers/gpio/gpio-dwapb.c
> > +++ b/drivers/gpio/gpio-dwapb.c
> > @@ -444,7 +444,7 @@ static void dwapb_configure_irqs(struct
> dwapb_gpio *gpio,
> > int i;
> >
> > for (i = 0; i < pp->ngpio; i++) {
> > -   if (pp->irq[i])
> > +   if (pp->irq[i] >= 0)
> > irq_set_chained_handler_and_data(pp-
> >irq[i],
> > dwapb_irq_handler, gpio);
> > }
> > @@ -562,7 +562,7 @@ dwapb_gpio_get_pdata(struct device *dev)
> > struct dwapb_platform_data *pdata;
> > struct dwapb_port_property *pp;
> > int nports;
> > -   int i;
> > +   int i, j;
> >
> > nports = device_get_child_node_count(dev);
> > if (nports == 0)
> > @@ -580,6 +580,8 @@ dwapb_gpio_get_pdata(struct device *dev)
> >
> > i = 0;
> > device_for_each_child_node(dev, fwnode)  {
> > +   struct device_node *np = NULL;
> > +
> > pp = >properties[i++];
> > pp->fwnode = fwnode;
> >
> > @@ -599,46 +601,35 @@ dwapb_gpio_get_pdata(struct device *dev)
> > pp->ngpio = 32;
> > }
> >
> > +   pp->irq_shared  = false;
> > +   pp->gpio_base   = -1;
> > +
> > /*
> >  * Only port A can provide interrupts in all configurations of
> >  * the IP.
> >  */
> > -   if (dev->of_node && pp->idx == 0 &&
> > -   fwnode_property_read_bool(fwnode,
> > - "interrupt-controller")) {
> > -   struct device_node *np = to_of_node(fwnode);
> > -   unsigned int j;
> > -
> > -   /*
> > -* The IP has configuration options to allow a single
> > -* combined interrupt or one per gpio. If one per
> gpio,
> > -* some might not be used.
> > -*/
> > -   for (j = 0; j < pp->ngpio; j++) {
> > -   int irq = of_irq_get(np, j);
> > -   if (irq < 0)
> > -   continue;
> > -
> > -   pp->irq[j] = irq;
> > -   pp->has_irq = true;
> > -   }
> > +   if (pp->idx != 0)
> > +   continue;
> >
> > -   if (!pp->has_irq)
> > -   dev_warn(dev, "no irq for port%d\n", pp-
> >idx);
> > +   if (dev->of_node && fwnode_property_read_bool(fwnode,
> > + "interrupt-controller")) {
> > +   np = to_of_node(fwnode);
> > }
> >
> > -   if (has_acpi_companion(dev) && pp->idx == 0) {
> > -   unsigned int j;
> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   pp->irq[j] = -ENXIO;
> >
> > -   for (j = 0; j < pp->ngpio; j++) {
> > +   if (np)
> > +   pp->irq[j] = of_irq_get(np, j);
> > +   else if (has_acpi_companion(dev))
> > pp->irq[j] =
> platform_get_irq(to_platform_device(dev), j);
> > -   if (pp->irq[j])
> > -   pp->has_irq = true;
> > -   }
> > +
> > +   if (pp->irq[j] >= 0)
> > +   pp->has_irq = true;
> > }
> >
> > -   pp->irq_shared  = false;
> > -   pp->gpio_base   = -1;
> > +   if (!pp->has_irq)
> > +   dev_warn(dev, "no irq for port%d\n", pp->idx);
> > }
> >
> > return pdata;
> > --
> > 2.7.4
> >

[PATCH] gpio: dwapb: Rework support for 1 interrupt per port A GPIO

[Phil Edworthy]

Treat DT and ACPI the same as much as possible. Note that we can't use
platform_get_irq() to get the DT interrupts as they are in the port
sub-node and hence do not have an associated platform device.

This also fixes a problem introduced with error checking when calling
platform_get_irq().

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 drivers/gpio/gpio-dwapb.c | 53 ---
 1 file changed, 22 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 7dcd06b..15b4154 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -444,7 +444,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
int i;
 
for (i = 0; i < pp->ngpio; i++) {
-   if (pp->irq[i])
+   if (pp->irq[i] >= 0)
irq_set_chained_handler_and_data(pp->irq[i],
dwapb_irq_handler, gpio);
}
@@ -562,7 +562,7 @@ dwapb_gpio_get_pdata(struct device *dev)
struct dwapb_platform_data *pdata;
struct dwapb_port_property *pp;
int nports;
-   int i;
+   int i, j;
 
nports = device_get_child_node_count(dev);
if (nports == 0)
@@ -580,6 +580,8 @@ dwapb_gpio_get_pdata(struct device *dev)
 
i = 0;
device_for_each_child_node(dev, fwnode)  {
+   struct device_node *np = NULL;
+
pp = >properties[i++];
pp->fwnode = fwnode;
 
@@ -599,46 +601,35 @@ dwapb_gpio_get_pdata(struct device *dev)
pp->ngpio = 32;
}
 
+   pp->irq_shared  = false;
+   pp->gpio_base   = -1;
+
/*
 * Only port A can provide interrupts in all configurations of
 * the IP.
 */
-   if (dev->of_node && pp->idx == 0 &&
-   fwnode_property_read_bool(fwnode,
- "interrupt-controller")) {
-   struct device_node *np = to_of_node(fwnode);
-   unsigned int j;
-
-   /*
-* The IP has configuration options to allow a single
-* combined interrupt or one per gpio. If one per gpio,
-* some might not be used.
-*/
-   for (j = 0; j < pp->ngpio; j++) {
-   int irq = of_irq_get(np, j);
-   if (irq < 0)
-   continue;
-
-   pp->irq[j] = irq;
-   pp->has_irq = true;
-   }
+   if (pp->idx != 0)
+   continue;
 
-   if (!pp->has_irq)
-   dev_warn(dev, "no irq for port%d\n", pp->idx);
+   if (dev->of_node && fwnode_property_read_bool(fwnode,
+ "interrupt-controller")) {
+   np = to_of_node(fwnode);
}
 
-   if (has_acpi_companion(dev) && pp->idx == 0) {
-   unsigned int j;
+   for (j = 0; j < pp->ngpio; j++) {
+   pp->irq[j] = -ENXIO;
 
-   for (j = 0; j < pp->ngpio; j++) {
+   if (np)
+   pp->irq[j] = of_irq_get(np, j);
+   else if (has_acpi_companion(dev))
pp->irq[j] = 
platform_get_irq(to_platform_device(dev), j);
-   if (pp->irq[j])
-   pp->has_irq = true;
-   }
+
+   if (pp->irq[j] >= 0)
+   pp->has_irq = true;
}
 
-   pp->irq_shared  = false;
-   pp->gpio_base   = -1;
+   if (!pp->has_irq)
+   dev_warn(dev, "no irq for port%d\n", pp->idx);
}
 
return pdata;
-- 
2.7.4

RE: [PATCH v6] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Linus,

On 23 May 2018 09:29, Linus Walleij wrote:
> On Fri, May 11, 2018 at 10:31 AM, Phil Edworthy wrote:
> 
> > The DesignWare GPIO IP can be configured for either 1 interrupt or 1
> > per GPIO in port A, but the driver currently only supports 1 interrupt.
> > See the DesignWare DW_apb_gpio Databook description of the
> > 'GPIO_INTR_IO' parameter.
> >
> > This change allows the driver to work with up to 32 interrupts, it
> > will get as many interrupts as specified in the DT 'interrupts' property.
> > It doesn't do anything clever with the different interrupts, it just
> > calls the same handler used for single interrupt hardware.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > Reviewed-by: Rob Herring <r...@kernel.org>
> > Acked-by: Lee Jones <lee.jo...@linaro.org>
> > ---
> > One point to mention is that I have made it possible for users to have
> > unconnected interrupts by specifying holes in the list of interrupts.
> > This is done by supporting the interrupts-extended DT prop.
> > However, I have no use for this and had to hack some test case for this.
> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?
> >
> > v6:
> >  - Treat DT and ACPI the same as much as possible. Note that we can't use
> >platform_get_irq() to get the DT interrupts as they are in the port
> >sub-node and hence do not have an associated platform device.
> 
> I already applied this patch in some version, can you check what is in my
> devel branch and send incremental patches on top if something needs
> changing?
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-
> gpio.git/commit/?h=devel=e6ca26abd37606ba4864f20c85d3fe4a2173b93f
> 
> Sorry for not knowing by heart what was applied or when, it's just too much
> for me sometimes.
No problem, I'll send a patch with the incremental changes.

Thanks
Phil

[PATCH v6] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
Reviewed-by: Rob Herring <r...@kernel.org>
Acked-by: Lee Jones <lee.jo...@linaro.org>
---
One point to mention is that I have made it possible for users to have
unconnected interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?

v6:
 - Treat DT and ACPI the same as much as possible. Note that we can't use
   platform_get_irq() to get the DT interrupts as they are in the port
   sub-node and hence do not have an associated platform device.
v5:
 - Rolled ACPI companion code provided by Hoan Tran into this patch.
v4:
 - Use of_irq_get() instead of of_irq_parse_one()+irq_create_of_mapping()
v3:
 - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems
v2:
 - Replaced interrupt-mask DT prop with support for the interrupts-extended
   prop. This means replacing the call to irq_of_parse_and_map() with calls
   to of_irq_parse_one() and irq_create_of_mapping().
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 +++-
 drivers/gpio/gpio-dwapb.c  | 49 +++---
 drivers/mfd/intel_quark_i2c_gpio.c |  3 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 4 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any are 
unconnected,
+  use the interrupts-extended property to specify the interrupts and set the
+  interrupt controller handle for unused interrupts to 0.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..15b4154 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i] >= 0)
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/* Ad

RE: [PATCH v5] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Andy,

On 05 May 2018 11:49 Andy Shevchenko wrote:
> On Thu, Apr 26, 2018 at 7:19 PM, Phil Edworthy wrote:
> 
> Sotty fo a late response. Consider follow up fixes for below.
> 
> > if (!pp->irq_shared) {
> > +   int i;
> > +
> > +   for (i = 0; i < pp->ngpio; i++) {
> > +   if (pp->irq[i])
> > +   irq_set_chained_handler_and_data(pp->irq[i],
> > +   dwapb_irq_handler, gpio);
> > +   }
> > } else {
> > /*
> >  * Request a shared IRQ since where MFD would have devices
> >  * using the same irq pin
> >  */
> > +   err = devm_request_irq(gpio->dev, pp->irq[0],
> >dwapb_irq_handler_mfd,
> >IRQF_SHARED, "gpio-dwapb-mfd",
> > gpio);
> 
> > +   if (pp->has_irq)
> > dwapb_configure_irqs(gpio, port, pp);
> 
> I would rather make irq array a type of signed int and move conditional into
> the function to test per IRQ based.
Ok, though since the driver can be used without interrupts, it has to check
if any are used before the driver does all the other interrupt related things.
 
> > /* Add GPIO-signaled ACPI event support */
> > +   if (pp->has_irq)
> > acpi_gpiochip_request_interrupts(>gc);
> 
> Perhaps something similar.
> 
> > if (dev->of_node && pp->idx == 0 &&
> > fwnode_property_read_bool(fwnode,
> >
> > "interrupt-controller")) {
> 
> > +   struct device_node *np = to_of_node(fwnode);
> > +   unsigned int j;
> > +
> > +   /*
> > +* The IP has configuration options to allow a 
> > single
> > +* combined interrupt or one per gpio. If one per 
> > gpio,
> > +* some might not be used.
> > +*/
> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   int irq = of_irq_get(np, j);
> > +   if (irq < 0)
> > +   continue;
> > +
> > +   pp->irq[j] = irq;
> > +   pp->has_irq = true;
> > +   }
> 
> for (...)
>  pp->irq = of_irq_get();
> 
> > }
> 
> > +   if (has_acpi_companion(dev) && pp->idx == 0) {
> > +   unsigned int j;
> > +
> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   pp->irq[j] = 
> > platform_get_irq(to_platform_device(dev), j);
> > +   if (pp->irq[j])
> > +   pp->has_irq = true;
> > +   }
> 
> Ditto.
> Moreover you have a bug here. See my proposal at the top of this message.
I guess you mean that it doesn’t check for errors returned?

> And now even better to ask, why platform_get_irq() wouldn't work for DT
> case?
The problem is that the interrupts are defined in the port sub-node in DT,
not in the gpio controller node, causing platform_get_irq() to fail. The
port sub-node doesn’t have an associated platform device. I don’t think
there is a way around this...

> > +
> > +   if (!pp->has_irq)
> > dev_warn(dev, "no irq for port%d\n",
> > pp->idx);
> 
> This could be issued in the actual function which will try to allocate IRQs
> (perhaps on debug level)
> 
> 
> P.S. Just think about it, perhaps you find even better solutions.
There's certainly some duplication between DT and ACPI that can be removed.

Thanks
Phil

RE: [PATCH] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Rob,

On 01 May 2018 14:29 Rob Herring wrote:
> On Mon, Apr 23, 2018 at 02:33:06PM +0100, Phil Edworthy wrote:
> > On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> > interrupts. All of these are passed to the GPIO IRQ Muxer, which
> > selects
> > 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> > aren't latched, so there is nothing to do in this driver when an
> > interrupt is received, other than tell the corresponding GPIO block.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> >  .../interrupt-controller/renesas,rzn1-mux.txt  |  85 ++
> 
> Please split bindings to a separate patch.
Will do.

> >  drivers/irqchip/Kconfig|  10 ++
> >  drivers/irqchip/Makefile   |   1 +
> >  drivers/irqchip/irq-rzn1-irq-mux.c | 178
> +
> >  4 files changed, 274 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mu
> > x.txt  create mode 100644 drivers/irqchip/irq-rzn1-irq-mux.c
> >
> > diff --git
> > a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-
> > mux.txt
> > b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-
> > mux.txt
> > new file mode 100644
> > index 000..f28a365
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,r
> > +++ zn1-mux.txt
> > @@ -0,0 +1,85 @@
> > +* Renesas RZ/N1 GPIO Interrupt Multiplexer
> > +
> > +On Renesas RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks
> > +each configured to have 32 interrupt outputs, so we have a total of
> > +96 GPIO interrupts. All of these are passed to the GPIO IRQ Muxer,
> > +which selects
> > +8 of the GPIO interrupts to pass onto the GIC.
> > +
> > +A single node in the device tree is used to describe the GPIO IRQ Muxer.
> > +
> > +Required properties:
> > +- compatible: the SoC specific name, i.e. "renesas,r9a06g032-gpioirq"
> > +   or "renesas,r9a06g033-gpioirq" followed by the SoC family name, i.e.
> > +   "renesas,rzn1-gpioirq".
> > +- interrupt-controller: Identifies the node as an interrupt controller.
> > +- #interrupt-cells: should be <1>. The meaning of the cells is the input
> > +   interrupt index, 0 to 95.
> > +- reg: Base address and size of GPIO IRQ Muxer registers.
> > +- interrupts: The list of interrupts generated by the muxer which are then
> > +   connected to a parent interrupt controller. The format of the interrupt
> > +   specifier depends in the interrupt parent controller.
> > +- gpioirq-#N: One property for each interrupt output from the GPIO IRQ
> Muxer
> > +   that specifies the input interrupt to use, #N is from 0 to 7.
> 
> Why do you need this in DT? Can't the driver handle this dynamically?
> When you request an interrupt on a GPIO line, then connect that GPIO line
> to a free IRQ line.
On the SoC that has this block, there is another CPU that runs firmware.
It's likely that the firmware will use some of these GPIO interrupts and
so we don't want them to move around. The firmware runs before Linux is
up, and luckily setting up the registers again won't affect the interrupts.

> If you really need this in DT, then interrupt-map can be used here.
Ok

> > +
> > +Optional properties:
> > +- interrupt-parent: pHandle of the parent interrupt controller, if not
> > +   inherited from the parent node.
> > +
> > +
> > +Example:
> > +
> > +   The following is an example for the RZ/N1D SoC.
> > +
> > +   gpioirq: gpioirq@51000480 {
> > +   compatible = "renesas,r9a06g032-gpioirq",
> > +   "renesas,rzn1-gpioirq";
> > +   reg = <0x51000480 0x20>;
> > +   interrupts =
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ,
> > +   ;
> > +   interrupt-controller;
> > +   #interrupt-cells = <1>;
> > +   status = "disabled";
> 
> Don't show status in examples.
Ok

> > +   };
> > +
> > +   gpio0: gpio@5000b000 {
> > +   compatible = "snps,dw-apb-gpio";
> > +   reg = <0x5000b000 0x80>;
> > +  

[PATCH v5] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

ACPI companion code provided by Hoan Tran <hot...@apm.com>. This was tested
on X-Gene by Hoan.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
Reviewed-by: Rob Herring <r...@kernel.org>
Acked-by: Lee Jones <lee.jo...@linaro.org>
---
One point to mention is that I have made it possible for users to have
unconncted interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?

v5:
 - Rolled ACPI companion code provided by Hoan Tran into this patch.
v4:
 - Use of_irq_get() instead of of_irq_parse_one()+irq_create_of_mapping()
v3:
 - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems
v2:
 - Replaced interrupt-mask DT prop with support for the interrupts-extended
   prop. This means replacing the call to irq_of_parse_and_map() with calls
   to of_irq_parse_one() and irq_create_of_mapping().

Note: There are a few *code* lines over 80 chars, but this is just guidance,
   right? Especially as there are already some lines over 80 chars.
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 -
 drivers/gpio/gpio-dwapb.c  | 46 +-
 drivers/mfd/intel_quark_i2c_gpio.c |  3 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 4 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any are 
unconnected,
+  use the interrupts-extended property to specify the interrupts and set the
+  interrupt controller handle for unused interrupts to 0.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..7dcd06b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i])
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/*

[PATCH] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
configured to have 32 interrupt outputs, so we have a total of 96 GPIO
interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
aren't latched, so there is nothing to do in this driver when an interrupt
is received, other than tell the corresponding GPIO block.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 .../interrupt-controller/renesas,rzn1-mux.txt  |  85 ++
 drivers/irqchip/Kconfig|  10 ++
 drivers/irqchip/Makefile   |   1 +
 drivers/irqchip/irq-rzn1-irq-mux.c | 178 +
 4 files changed, 274 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
 create mode 100644 drivers/irqchip/irq-rzn1-irq-mux.c

diff --git 
a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt 
b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
new file mode 100644
index 000..f28a365
--- /dev/null
+++ 
b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
@@ -0,0 +1,85 @@
+* Renesas RZ/N1 GPIO Interrupt Multiplexer
+
+On Renesas RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
+configured to have 32 interrupt outputs, so we have a total of 96 GPIO
+interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
+8 of the GPIO interrupts to pass onto the GIC.
+
+A single node in the device tree is used to describe the GPIO IRQ Muxer.
+
+Required properties:
+- compatible: the SoC specific name, i.e. "renesas,r9a06g032-gpioirq"
+   or "renesas,r9a06g033-gpioirq" followed by the SoC family name, i.e.
+   "renesas,rzn1-gpioirq".
+- interrupt-controller: Identifies the node as an interrupt controller.
+- #interrupt-cells: should be <1>. The meaning of the cells is the input
+   interrupt index, 0 to 95.
+- reg: Base address and size of GPIO IRQ Muxer registers.
+- interrupts: The list of interrupts generated by the muxer which are then
+   connected to a parent interrupt controller. The format of the interrupt
+   specifier depends in the interrupt parent controller.
+- gpioirq-#N: One property for each interrupt output from the GPIO IRQ Muxer
+   that specifies the input interrupt to use, #N is from 0 to 7.
+
+Optional properties:
+- interrupt-parent: pHandle of the parent interrupt controller, if not
+   inherited from the parent node.
+
+
+Example:
+
+   The following is an example for the RZ/N1D SoC.
+
+   gpioirq: gpioirq@51000480 {
+   compatible = "renesas,r9a06g032-gpioirq",
+   "renesas,rzn1-gpioirq";
+   reg = <0x51000480 0x20>;
+   interrupts =
+   ,
+   ,
+   ,
+   ,
+   ,
+   ,
+   ,
+   ;
+   interrupt-controller;
+   #interrupt-cells = <1>;
+   status = "disabled";
+   };
+
+   gpio0: gpio@5000b000 {
+   compatible = "snps,dw-apb-gpio";
+   reg = <0x5000b000 0x80>;
+   #address-cells = <1>;
+   #size-cells = <0>;
+   clock-names = "bus";
+   clocks = <_gpio0>;
+   status = "disabled";
+
+   gpio0a: gpio-controller@0 {
+   compatible = "snps,dw-apb-gpio-port";
+   bank-name = "gpio0a";
+   gpio-controller;
+   #gpio-cells = <2>;
+   snps,nr-gpios = <32>;
+   reg = <0>;
+
+   interrupt-controller;
+   interrupt-parent = <>;
+   interrupts =   < 0  1  2  3  4  5  6  7
+8  9 10 11 12 13 14 15
+   16 17 18 19 20 21 22 23
+   24 25 26 27 28 29 30 31 >;
+   #interrupt-cells = <2>;
+   };
+   };
+
+
+   The following is an example for a board using this.
+
+{
+   status = "okay";
+   gpioirq-0 = <24>;   /* gpio0a 24 */
+   gpioirq-4 = <3>;/* gpio0a 3 */
+   };
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index e9233db..16f2633 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -204,6 +204,16 @@ config RENESAS_IRQC
select GENERIC_IRQ_CHIP
select IRQ_DOMAIN
 
+config RENESAS_RZN1_IRQ_MUX

RE: [PATCH v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Hoan

On 18 April 2018 08:03 Hoan Tran wrote:
> On Fri, Apr 13, 2018 at 9:47 AM, Phil Edworthy wrote:
> > On 13 April 2018 17:37 Hoan Tran wrote:
> >> On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote:
> >> > The DesignWare GPIO IP can be configured for either 1 interrupt or
> >> > 1 per GPIO in port A, but the driver currently only supports 1 interrupt.
> >> > See the DesignWare DW_apb_gpio Databook description of the
> >> > 'GPIO_INTR_IO' parameter.
> >> >
> >> > This change allows the driver to work with up to 32 interrupts, it
> >> > will get as many interrupts as specified in the DT 'interrupts' property.
> >> > It doesn't do anything clever with the different interrupts, it
> >> > just calls the same handler used for single interrupt hardware.
> >> >
> >> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> >> > ---
> >> > One point to mention is that I have made it possible for users to
> >> > have unconncted interrupts by specifying holes in the list of interrupts.
> >> > This is done by supporting the interrupts-extended DT prop.
> >> > However, I have no use for this and had to hack some test case for this.
> >> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?
> >> >
> >> > v3:
> >> >  - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid
> >> > bisect problems
> >> > v2:
> >> >  - Replaced interrupt-mask DT prop with support for the interrupts-
> >> extended
> >> >prop. This means replacing the call to irq_of_parse_and_map() with
> calls
> >> >to of_irq_parse_one() and irq_create_of_mapping().
> >> >
> >> > Note: There are a few *code* lines over 80 chars, but this is just
> guidance,
> >> >right? Especially as there are already some lines over 80 chars.
> >> > ---
> > [snip]
> >
> >> > -   if (has_acpi_companion(dev) && pp->idx == 0)
> >> > -   pp->irq = 
> >> > platform_get_irq(to_platform_device(dev), 0);
> >> > +   if (has_acpi_companion(dev) && pp->idx == 0) {
> >> > +   pp->irq[0] = 
> >> > platform_get_irq(to_platform_device(dev),
> 0);
> >> > +   if (pp->irq[0])
> >> > +   pp->has_irq = true;
> >> > +   }
> >>
> >> It doesn't work for ACPI. Could you do the same logic for ACPI?
> > I don’t have access to any device that was baked (i.e. fabbed) with
> > multiple output interrupts from the Synopsys GPIO blocks and use ACPI.
> > I don't know if any such device exists.
> 
> Below code is tested on X-Gene system which supports 1 interrupt per GPIO
> on Port A. You can update it into your patch.
> 
> -   if (has_acpi_companion(dev) && pp->idx == 0)
> -   pp->irq = platform_get_irq(to_platform_device(dev), 
> 0);
> +   if (has_acpi_companion(dev) && pp->idx == 0) {
> +   unsigned int j;
> +   for (j = 0; j < pp->ngpio; j++) {
> +   pp->irq[j] =
> platform_get_irq(to_platform_device(dev), j);
> +   if (pp->irq[j])
> +   pp->has_irq = true;
> +   }
> +   }
Since I've already got some reviewed-by and acks for v4, I'll leave it to Linus
to decide if he wants me to roll your changes into this patch or for you to
submit a separate patch.

Thanks
Phil


> >> > pp->irq_shared  = false;
> >> > pp->gpio_base   = -1;
> >> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
> >> > b/drivers/mfd/intel_quark_i2c_gpio.c
> >> > index 90e35de..5bddb84 100644
> >> > --- a/drivers/mfd/intel_quark_i2c_gpio.c
> >> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> >> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct
> >> > pci_dev
> >> *pdev, struct mfd_cell *cell)
> >> > pdata->properties->idx  = 0;
> >> > pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
> >> > pdata->properties->gpio_base=
> INTEL_QUARK_MFD_GPIO_BASE;
> >> > -   pdata->properties->irq  = pdev->irq;
> >> > +   pdata->properties->irq[0]   = pdev->irq;
> >> > +   pdata->properties->has_irq  = true;
> >> > pdata->properties->irq_shared   = true;
> >> >
> >> > cell->platform_data = pdata; diff --git
> >> > a/include/linux/platform_data/gpio-dwapb.h
> >> > b/include/linux/platform_data/gpio-dwapb.h
> >> > index 2dc7f4a..5a52d69 100644
> >> > --- a/include/linux/platform_data/gpio-dwapb.h
> >> > +++ b/include/linux/platform_data/gpio-dwapb.h
> >> > @@ -19,7 +19,8 @@ struct dwapb_port_property {
> >> > unsigned intidx;
> >> > unsigned intngpio;
> >> > unsigned intgpio_base;
> >> > -   unsigned intirq;
> >> > +   unsigned intirq[32];
> >> > +   boolhas_irq;
> >> > boolirq_shared;
> >> >  };
> >> >
> >> > --
> >> > 2.7.4
> >> >

[PATCH v4] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
One point to mention is that I have made it possible for users to have
unconncted interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?

v4:
 - Use of_irq_get() instead of of_irq_parse_one()+irq_create_of_mapping()
v3:
 - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems
v2:
 - Replaced interrupt-mask DT prop with support for the interrupts-extended
   prop. This means replacing the call to irq_of_parse_and_map() with calls
   to of_irq_parse_one() and irq_create_of_mapping().

Note: There are a few *code* lines over 80 chars, but this is just guidance,
   right? Especially as there are already some lines over 80 chars.

snps:gpio fix

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 +++--
 drivers/gpio/gpio-dwapb.c  | 42 +-
 drivers/mfd/intel_quark_i2c_gpio.c |  3 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 4 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any are 
unconnected,
+  use the interrupts-extended property to specify the interrupts and set the
+  interrupt controller handle for unused interrupts to 0.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..7a1022f 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i])
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/* Add GPIO-signaled ACPI event support */
-   if (pp->irq)
+   if (pp->has_irq)
acpi_gpiochip_request_interrupts(>gc);
 
return err;
@@ -601,13 +606,32 @@ dw

RE: [PATCH v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Rob,

On 16 April 2018 21:03 Rob Herring wrote:
> On Fri, Apr 13, 2018 at 09:51:12AM +0100, Phil Edworthy wrote:
> > The DesignWare GPIO IP can be configured for either 1 interrupt or 1
> > per GPIO in port A, but the driver currently only supports 1 interrupt.
> > See the DesignWare DW_apb_gpio Databook description of the
> > 'GPIO_INTR_IO' parameter.
> >
> > This change allows the driver to work with up to 32 interrupts, it
> > will get as many interrupts as specified in the DT 'interrupts' property.
> > It doesn't do anything clever with the different interrupts, it just
> > calls the same handler used for single interrupt hardware.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> > One point to mention is that I have made it possible for users to have
> > unconncted interrupts by specifying holes in the list of interrupts.
> > This is done by supporting the interrupts-extended DT prop.
> > However, I have no use for this and had to hack some test case for this.
> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?
> >
> > v3:
> >  - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid
> > bisect problems
> > v2:
> >  - Replaced interrupt-mask DT prop with support for the interrupts-
> extended
> >prop. This means replacing the call to irq_of_parse_and_map() with calls
> >to of_irq_parse_one() and irq_create_of_mapping().
> >
> > Note: There are a few *code* lines over 80 chars, but this is just guidance,
> >right? Especially as there are already some lines over 80 chars.
> > ---
[snip]

> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   if (of_irq_parse_one(np, j, ))
> > +   continue;
> > +
> > +   pp->irq[j] = irq_create_of_mapping();
> 
> I'm hoping to not have new users of of_irq_parse_one and
> irq_create_of_mapping. Can you use of_irq_get instead? It will base back
> error codes so you can distinguish different conditions.
Sure, I hadn't noticed that particular variant!

Thanks
Phil

> > +   if (pp->irq[j])
> > +   pp->has_irq = true;
> > +   }
> > +
> > +   if (!pp->has_irq)
> > dev_warn(dev, "no irq for port%d\n", pp-
> >idx);
> > }
> >
> > -   if (has_acpi_companion(dev) && pp->idx == 0)
> > -   pp->irq =
> platform_get_irq(to_platform_device(dev), 0);
> > +   if (has_acpi_companion(dev) && pp->idx == 0) {
> > +   pp->irq[0] =
> platform_get_irq(to_platform_device(dev), 0);
> > +   if (pp->irq[0])
> > +   pp->has_irq = true;
> > +   }
> >
> > pp->irq_shared  = false;
> > pp->gpio_base   = -1;

RE: [PATCH v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

Hi Hoan,

On 13 April 2018 17:37 Hoan Tran wrote:
> On Fri, Apr 13, 2018 at 1:51 AM, Phil Edworthy wrote:
> > The DesignWare GPIO IP can be configured for either 1 interrupt or 1
> > per GPIO in port A, but the driver currently only supports 1 interrupt.
> > See the DesignWare DW_apb_gpio Databook description of the
> > 'GPIO_INTR_IO' parameter.
> >
> > This change allows the driver to work with up to 32 interrupts, it
> > will get as many interrupts as specified in the DT 'interrupts' property.
> > It doesn't do anything clever with the different interrupts, it just
> > calls the same handler used for single interrupt hardware.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> > One point to mention is that I have made it possible for users to have
> > unconncted interrupts by specifying holes in the list of interrupts.
> > This is done by supporting the interrupts-extended DT prop.
> > However, I have no use for this and had to hack some test case for this.
> > Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?
> >
> > v3:
> >  - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid
> > bisect problems
> > v2:
> >  - Replaced interrupt-mask DT prop with support for the interrupts-
> extended
> >prop. This means replacing the call to irq_of_parse_and_map() with calls
> >to of_irq_parse_one() and irq_create_of_mapping().
> >
> > Note: There are a few *code* lines over 80 chars, but this is just guidance,
> >right? Especially as there are already some lines over 80 chars.
> > ---
[snip]

> > -   if (has_acpi_companion(dev) && pp->idx == 0)
> > -   pp->irq = platform_get_irq(to_platform_device(dev), 
> > 0);
> > +   if (has_acpi_companion(dev) && pp->idx == 0) {
> > +   pp->irq[0] = 
> > platform_get_irq(to_platform_device(dev), 0);
> > +   if (pp->irq[0])
> > +   pp->has_irq = true;
> > +   }
> 
> It doesn't work for ACPI. Could you do the same logic for ACPI?
I don’t have access to any device that was baked (i.e. fabbed) with multiple
output interrupts from the Synopsys GPIO blocks and use ACPI. I don't
know if any such device exists.

I would prefer not writing code that can be tested easily. I cannot even
test the current, albeit small, changes to the Intel Quark MFD.

Regards
Phil

> Thanks
> Hoan
> 
> >
> > pp->irq_shared  = false;
> > pp->gpio_base   = -1;
> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
> > b/drivers/mfd/intel_quark_i2c_gpio.c
> > index 90e35de..5bddb84 100644
> > --- a/drivers/mfd/intel_quark_i2c_gpio.c
> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev
> *pdev, struct mfd_cell *cell)
> > pdata->properties->idx  = 0;
> > pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
> > pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
> > -   pdata->properties->irq  = pdev->irq;
> > +   pdata->properties->irq[0]   = pdev->irq;
> > +   pdata->properties->has_irq  = true;
> > pdata->properties->irq_shared   = true;
> >
> > cell->platform_data = pdata;
> > diff --git a/include/linux/platform_data/gpio-dwapb.h
> > b/include/linux/platform_data/gpio-dwapb.h
> > index 2dc7f4a..5a52d69 100644
> > --- a/include/linux/platform_data/gpio-dwapb.h
> > +++ b/include/linux/platform_data/gpio-dwapb.h
> > @@ -19,7 +19,8 @@ struct dwapb_port_property {
> > unsigned intidx;
> > unsigned intngpio;
> > unsigned intgpio_base;
> > -   unsigned intirq;
> > +   unsigned intirq[32];
> > +   boolhas_irq;
> > boolirq_shared;
> >  };
> >
> > --
> > 2.7.4
> >

[PATCH v3] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
One point to mention is that I have made it possible for users to have
unconncted interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?

v3:
 - Rolled mfd: intel_quark_i2c_gpio fix into this patch to avoid bisect problems
v2:
 - Replaced interrupt-mask DT prop with support for the interrupts-extended
   prop. This means replacing the call to irq_of_parse_and_map() with calls
   to of_irq_parse_one() and irq_create_of_mapping().

Note: There are a few *code* lines over 80 chars, but this is just guidance,
   right? Especially as there are already some lines over 80 chars.
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 -
 drivers/gpio/gpio-dwapb.c  | 43 +-
 drivers/mfd/intel_quark_i2c_gpio.c |  3 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 4 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any are 
unconnected,
+  use the interrupts-extended property to specify the interrupts and set the
+  interrupt controller handle for unused interrupts to 0.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..3273504 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i])
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/* Add GPIO-signaled ACPI event support */
-   if (pp->irq)
+   if (pp->has_irq)
acpi_gpiochip_request_interrupts(>gc);
 
return err;
@@ -601,13 +606,33 @@ dwapb_gpio_get_pdata(struct device *dev)
if (dev->of_node && pp->idx == 0 &&
fwnode_

RE: [PATCH v2 2/2] mfd: intel_quark_i2c_gpio: Update Synopsys GPIO interrupts

[Phil Edworthy]

Hi Geert,

On 13 April 2018 09:20 Geert Uytterhoeven wrote:
> On Fri, Apr 13, 2018 at 10:08 AM, Phil Edworthy wrote:
> > Since the way the Synopsys GPIO interrupts are stored has changed,
> > this driver needs to be updated in line with the changes.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> > v2:
> >  - New patch in v2 to fix the only other user of struct
> dwapb_port_property.
> 
> Thanks for your patch!
> 
> To avoid bisection compile failures due to the changed type of
> dwapb_port_property.irq, I think this should be folded into the first patch,
Right, thanks for pointing this out!
Phil

> > ---
> >  drivers/mfd/intel_quark_i2c_gpio.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/intel_quark_i2c_gpio.c
> > b/drivers/mfd/intel_quark_i2c_gpio.c
> > index 90e35de..5bddb84 100644
> > --- a/drivers/mfd/intel_quark_i2c_gpio.c
> > +++ b/drivers/mfd/intel_quark_i2c_gpio.c
> > @@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev
> *pdev, struct mfd_cell *cell)
> > pdata->properties->idx  = 0;
> > pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
> > pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
> > -   pdata->properties->irq  = pdev->irq;
> > +   pdata->properties->irq[0]   = pdev->irq;
> > +   pdata->properties->has_irq  = true;
> > pdata->properties->irq_shared   = true;
> >
> > cell->platform_data = pdata;
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

[PATCH v2 0/2] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

All details about the change is in patch 0001. patch 0002 fixes up other users
of 'struct dwapb_port_property'.

One point to mention is that I have made it possible for users to have
unconncted interrupts by specifying holes in the list of interrupts. This is
done by supporting the interrupts-extended DT prop.
However, I have no use for this and had to hack some test case for this.
Perhaps the driver should support 1 interrupt or all GPIOa as interrupts?

Phil Edworthy (2):
  gpio: dwapb: Add support for 1 interrupt per port A GPIO
  mfd: intel_quark_i2c_gpio: Update Synopsys GPIO interrupts

 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 -
 drivers/gpio/gpio-dwapb.c  | 43 +-
 drivers/mfd/intel_quark_i2c_gpio.c |  3 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 4 files changed, 45 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] gpio: dwapb: Add support for 1 interrupt per port A GPIO

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 interrupt or 1
per GPIO in port A, but the driver currently only supports 1 interrupt.
See the DesignWare DW_apb_gpio Databook description of the
'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v2:
 - Replaced interrupt-mask DT prop with support for the interrupts-extended
   prop. This means replacing the call to irq_of_parse_and_map() with calls
   to of_irq_parse_one() and irq_create_of_mapping().

Note: There are a few *code* lines over 80 chars, but this is just guidance,
   right? Especially as there are already some lines over 80 chars.
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   |  9 -
 drivers/gpio/gpio-dwapb.c  | 43 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 3 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..3c1118b 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,13 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any are 
unconnected,
+  use the interrupts-extended property to specify the interrupts and set the
+  interrupt controller handle for unused interrupts to 0.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..3273504 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i])
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/* Add GPIO-signaled ACPI event support */
-   if (pp->irq)
+   if (pp->has_irq)
acpi_gpiochip_request_interrupts(>gc);
 
return err;
@@ -601,13 +606,33 @@ dwapb_gpio_get_pdata(struct device *dev)
if (dev->of_node && pp->idx == 0 &&
fwnode_property_read_bool(fwnode,
  "interrupt-controller")) {
-   pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
-   if (!pp->irq)
+   struct device_node *np = to_of_node(fwnode);
+   struct of_phandle_args oirq;
+   unsigned int j;
+
+   /*
+* The IP has configuration options to allow a sing

[PATCH v2 2/2] mfd: intel_quark_i2c_gpio: Update Synopsys GPIO interrupts

[Phil Edworthy]

Since the way the Synopsys GPIO interrupts are stored has changed, this
driver needs to be updated in line with the changes.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v2:
 - New patch in v2 to fix the only other user of struct dwapb_port_property.
---
 drivers/mfd/intel_quark_i2c_gpio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/intel_quark_i2c_gpio.c 
b/drivers/mfd/intel_quark_i2c_gpio.c
index 90e35de..5bddb84 100644
--- a/drivers/mfd/intel_quark_i2c_gpio.c
+++ b/drivers/mfd/intel_quark_i2c_gpio.c
@@ -233,7 +233,8 @@ static int intel_quark_gpio_setup(struct pci_dev *pdev, 
struct mfd_cell *cell)
pdata->properties->idx  = 0;
pdata->properties->ngpio= INTEL_QUARK_MFD_NGPIO;
pdata->properties->gpio_base= INTEL_QUARK_MFD_GPIO_BASE;
-   pdata->properties->irq  = pdev->irq;
+   pdata->properties->irq[0]   = pdev->irq;
+   pdata->properties->has_irq  = true;
pdata->properties->irq_shared   = true;
 
cell->platform_data = pdata;
-- 
2.7.4

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Andy,

On 05 April 2018 10:43, Phil Edworthy wrote:
> On 30 March 2018 22:26 Andy Shevchenko wrote:
> > On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote:
> > > The DesignWare GPIO IP can be configured for either 1 or 32
> > > interrupts,
> >
> > 1 to 32, or just a choice between two?
> Just a choice of 1 or 32.
Sorry, I was wrong about this... the manual does not say 1 or 32. It says:
"Port A can be configured to generate multiple interrupt signals or
a single combined interrupt flag. When set to 1, the component generates a
single combined interrupt flag."

There is no other text describing this option, but I believe all GPIOs on
port A will have an interrupt. In our case we have 32 GPIOs on port A
and 32 interrupts connected to them.

Thanks
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Geert,

On 10 April 2018 15:29 Geert Uytterhoeven wrote:
> On Tue, Apr 10, 2018 at 4:23 PM, Phil Edworthy wrote:
> > On 10 April 2018 07:24 Phil Edworthy wrote:
> >> On 09 April 2018 20:20 Rob Herring wrote:
> >> > On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote:
> > [...]
> >> > > +- interrupt-mask : a 32-bit bit mask that specifies which
> >> > > +interrupts in the list
> >> > > +  of interrupts is valid, bit is 1 for a valid irq.
> >> >
> >> > This is not a standard property and would need a vendor prefix.
> >> > However,
> >> I'd
> >> > prefer you just skip any not connected interrupts with an invalid
> >> > interrupt number. Then the GPIO number is the index into "interrupts".
> >> Makes sense, I'll rework it to do this.
> > Err, what would an invalid interrupt number be?
> > If I use -1, I get a DT parsing error and 0 is certainly valid. If the
> > number is larger than the valid interrupt range I get errors during probe.
> 
> Perhaps using interrupts-extended instead of interrupts?
> 
> E.g.
> 
> interrupts-extended = < 5 1>, <0>, < 1 0>;

Thanks for the pointer, I'll have a look.
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Rob,

On 10 April 2018 07:24 Phil Edworthy wrote:
> On 09 April 2018 20:20 Rob Herring wrote:
> > On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote:
[...]
> > > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts
> > > +in the list
> > > +  of interrupts is valid, bit is 1 for a valid irq.
> >
> > This is not a standard property and would need a vendor prefix. However,
> I'd
> > prefer you just skip any not connected interrupts with an invalid interrupt
> > number. Then the GPIO number is the index into "interrupts".
> Makes sense, I'll rework it to do this.
Err, what would an invalid interrupt number be?
If I use -1, I get a DT parsing error and 0 is certainly valid. If the number is
larger than the valid interrupt range I get errors during probe.

Thanks
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Rob,

On 09 April 2018 20:20 Rob Herring wrote:
> On Wed, Mar 28, 2018 at 03:22:30PM +0100, Phil Edworthy wrote:
> > The DesignWare GPIO IP can be configured for either 1 or 32
> > interrupts, but the driver currently only supports 1 interrupt. See
> > the DesignWare DW_apb_gpio Databook description of the
> 'GPIO_INTR_IO' parameter.
> 
> Someday h/w designers will realize this does nothing to optimize interrupt
> handling...
I can imagine some software where the isr is written to handle a specific GPIO
interrupt _could_ be faster, though no sane software would be designed like
that.

> > This change allows the driver to work with up to 32 interrupts, it
> > will get as many interrupts as specified in the DT 'interrupts' property.
> > It doesn't do anything clever with the different interrupts, it just
> > calls the same handler used for single interrupt hardware.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> > Note: There are a few lines over 80 chars, but this is just guidance, right?
> >   Especially as there are already some lines over 80 chars.
> 
> Code, yes, but not for paragraphs of text in DT bindings.
Good, that's what I did.

> > ---
> >  .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   | 10 -
> >  drivers/gpio/gpio-dwapb.c  | 44 
> > +-
> >  include/linux/platform_data/gpio-dwapb.h   |  3 +-
> >  3 files changed, 45 insertions(+), 12 deletions(-)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> > b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> > index 4a75da7..e343581 100644
> > --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> > +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> > @@ -26,8 +26,14 @@ controller.
> >the second encodes the triger flags encoded as described in
> >
> > Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >  - interrupt-parent : The parent interrupt controller.
> > -- interrupts : The interrupt to the parent controller raised when
> > GPIOs
> > -  generate the interrupts.
> > +- interrupts : The interrupts to the parent controller raised when
> > +GPIOs
> > +  generate the interrupts. If the controller provides one combined
> > +interrupt
> > +  for all GPIOs, specify a single interrupt. If the controller
> > +provides one
> > +  interrupt for each GPIO, provide a list of interrupts that
> > +correspond to each
> > +  of the GPIO pins. When specifying multiple interrupts, if any of
> > +the GPIOs are
> > +  not connected to an interrupt, use the interrupt-mask property.
> > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts
> > +in the list
> > +  of interrupts is valid, bit is 1 for a valid irq.
> 
> This is not a standard property and would need a vendor prefix. However, I'd
> prefer you just skip any not connected interrupts with an invalid interrupt
> number. Then the GPIO number is the index into "interrupts".
Makes sense, I'll rework it to do this.

> >  - snps,nr-gpios : The number of pins in the port, a single cell.
> 
> This BTW should be deprecated to use "nr-gpios" instead, but that's another
> patch.

Thanks for your comments,
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Geert,

On 06 April 2018 10:57 Geert Uytterhoeven wrote:
> On Thu, Apr 5, 2018 at 11:42 AM, Phil Edworthy wrote:
> > On 30 March 2018 22:26 Andy Shevchenko wrote:
> >> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote:
> >> > The DesignWare GPIO IP can be configured for either 1 or 32
> >> > interrupts,
> >>
> >> 1 to 32, or just a choice between two?
> > Just a choice of 1 or 32.
> > Note that by 'configured' I am talking about the hardware being
> > configured in RTL prior to manufacturing a device. Once made, you cannot
> change it.
> > This configuration affects the number of output interrupt signals from
> > the GPIO Controller block that are connected to an interrupt controller.
> 
> Differentiating between different versions of an IP block using DT properties
> is usually a bad idea, for several reasons:
>   - What if you discover another difference later?
>   - You cannot add differentiating properties retroactively, because of
> backwards
>  compatibility with old DTBS.
> 
> Hence I think you should introduce a new compatible value instead.

This is not a different version of the IP, just a different configuration 
option.
Most IP blocks have a huge number of knobs that can be twiddled by the HW
people, such as cache size, UART fifo depth. I think this is no different.

Thanks
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi Andy,

On 30 March 2018 22:26 Andy Shevchenko wrote:
> On Wed, Mar 28, 2018 at 5:22 PM, Phil Edworthy wrote:
> > The DesignWare GPIO IP can be configured for either 1 or 32
> > interrupts,
> 
> 1 to 32, or just a choice between two?
Just a choice of 1 or 32.
Note that by 'configured' I am talking about the hardware being configured in
RTL prior to manufacturing a device. Once made, you cannot change it.
This configuration affects the number of output interrupt signals from the GPIO
Controller block that are connected to an interrupt controller.

> > but the driver currently only supports 1 interrupt. See the DesignWare
> > DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.
> 
> Will see after holiday and perhaps make more comments. Here is just a brief
> review.
> 
> > +- interrupts : The interrupts to the parent controller raised when
> > +GPIOs
> > +  generate the interrupts. If the controller provides one combined
> > +interrupt
> > +  for all GPIOs, specify a single interrupt. If the controller
> > +provides one
> > +  interrupt for each GPIO, provide a list of interrupts that
> > +correspond to each
> > +  of the GPIO pins. When specifying multiple interrupts, if any of
> > +the GPIOs are
> > +  not connected to an interrupt, use the interrupt-mask property.
> > +- interrupt-mask : a 32-bit bit mask that specifies which interrupts
> > +in the list
> > +  of interrupts is valid, bit is 1 for a valid irq.
> 
> So, but why one will need that in practice? GPIO driver usually provides a pin
> based IRQ chip which maps each pin to the corresponding offset inside
> specific IRQ domain.
On an ARM device we have this GPIO block connected to the GIC interrupt
controller, i.e. the Synopsys GPIO controller interrupts can* have a 1 to 1
mapping to the GIC interrupts. At the moment, the GPIO driver only allows a
single irq signal to specified.
* this is not strictly accurate on the device I am working on, there is another
block of IP between the two, but that doesn't matter in this case.

> > +   struct device_node *np = to_of_node(fwnode);
> > +   u32 irq_mask = 0x;
> 
> Why? Shouldn't it be dependent to the amount of actual pins / ports?
> Intel Quark has only 8 AFAIR.
It's just a default which can be overridden via device tree.
For Quark, since you currently only use a single irq, I guess the HW was
configured that way. In which case, you wouldn't use any of this.

> > +   int j;
> > +
> > +   /* Optional irq mask */
> > +   fwnode_property_read_u32(fwnode,
> > + "interrupt-mask", _mask);
> > +
> > +   /*
> > +* The IP has configuration options to allow a 
> > single
> > +* combined interrupt or one per gpio. If one per 
> > gpio,
> > +* some might not be used.
> > +*/
> 
> > +   for (j = 0; j < pp->ngpio; j++) {
> > +   if (irq_mask & BIT(j)) {
> 
> for_each_set_bit() is in kernel for ages!
There's lot of stuff in the kernel for ages that I can't remember!
I'll fix this :)

> > +   pp->irq[j] = 
> > irq_of_parse_and_map(np, j);
> > +   if (pp->irq[j])
> > +   pp->has_irq = true;
> > +   }
> > +   }
> 
> 
> So, on the first glance the patch looks either superfluous or taking wrong
> approach. Please, elaborate more why it's done in this way and what the
> case for all this in practice.
Hopefully I have explained it a bit better above.

Thanks for your comments
Phil

RE: [PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

Hi,

On 28 March 2018 15:23, Phil Edworthy wrote:
> The DesignWare GPIO IP can be configured for either 1 or 32 interrupts,
> but the driver currently only supports 1 interrupt. See the DesignWare
> DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.
> 
> This change allows the driver to work with up to 32 interrupts, it will
> get as many interrupts as specified in the DT 'interrupts' property.
> It doesn't do anything clever with the different interrupts, it just calls
> the same handler used for single interrupt hardware.
> 
> Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> ---
> Note: There are a few lines over 80 chars, but this is just guidance, right?
>   Especially as there are already some lines over 80 chars.
> ---
>  .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   | 10 -
>  drivers/gpio/gpio-dwapb.c  | 44 
> +-
>  include/linux/platform_data/gpio-dwapb.h   |  3 +-
>  3 files changed, 45 insertions(+), 12 deletions(-)

This patch triggers a build error for Quark MFD driver, which is the only user
of the structure outside of the driver. I will fix that with an additional 
patch,
but I'll wait to see what other comments I get first.

Thanks
Phil


> diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> index 4a75da7..e343581 100644
> --- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> +++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
> @@ -26,8 +26,14 @@ controller.
>the second encodes the triger flags encoded as described in
>Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>  - interrupt-parent : The parent interrupt controller.
> -- interrupts : The interrupt to the parent controller raised when GPIOs
> -  generate the interrupts.
> +- interrupts : The interrupts to the parent controller raised when GPIOs
> +  generate the interrupts. If the controller provides one combined interrupt
> +  for all GPIOs, specify a single interrupt. If the controller provides one
> +  interrupt for each GPIO, provide a list of interrupts that correspond to 
> each
> +  of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs
> are
> +  not connected to an interrupt, use the interrupt-mask property.
> +- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the 
> list
> +  of interrupts is valid, bit is 1 for a valid irq.
>  - snps,nr-gpios : The number of pins in the port, a single cell.
>  - resets : Reset line for the controller.
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 226977f..47d82f9 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct
> dwapb_gpio *gpio,
>   irq_gc->chip_types[1].handler = handle_edge_irq;
> 
>   if (!pp->irq_shared) {
> - irq_set_chained_handler_and_data(pp->irq,
> dwapb_irq_handler,
> -  gpio);
> + int i;
> +
> + for (i = 0; i < pp->ngpio; i++) {
> + if (pp->irq[i])
> + irq_set_chained_handler_and_data(pp-
> >irq[i],
> + dwapb_irq_handler, gpio);
> + }
>   } else {
>   /*
>* Request a shared IRQ since where MFD would have
> devices
>* using the same irq pin
>*/
> - err = devm_request_irq(gpio->dev, pp->irq,
> + err = devm_request_irq(gpio->dev, pp->irq[0],
>  dwapb_irq_handler_mfd,
>  IRQF_SHARED, "gpio-dwapb-mfd", gpio);
>   if (err) {
> @@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio
> *gpio,
>   if (pp->idx == 0)
>   port->gc.set_config = dwapb_gpio_set_config;
> 
> - if (pp->irq)
> + if (pp->has_irq)
>   dwapb_configure_irqs(gpio, port, pp);
> 
>   err = gpiochip_add_data(>gc, port);
> @@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio
> *gpio,
>   port->is_registered = true;
> 
>   /* Add GPIO-signaled ACPI event support */
> - if (pp->irq)
> + if (pp->has_irq)
>   acpi_gpiochip_request_interrupts(>gc);
> 
>   return err;
> @@ -601,13 +606,34 @@ dwapb_gpio_get_pdata(struct device *dev)
>   if (dev->of_node && pp->idx

[PATCH] gpio: dwapb: Add support for 32 interrupts

[Phil Edworthy]

The DesignWare GPIO IP can be configured for either 1 or 32 interrupts,
but the driver currently only supports 1 interrupt. See the DesignWare
DW_apb_gpio Databook description of the 'GPIO_INTR_IO' parameter.

This change allows the driver to work with up to 32 interrupts, it will
get as many interrupts as specified in the DT 'interrupts' property.
It doesn't do anything clever with the different interrupts, it just calls
the same handler used for single interrupt hardware.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
Note: There are a few lines over 80 chars, but this is just guidance, right?
  Especially as there are already some lines over 80 chars.
---
 .../devicetree/bindings/gpio/snps-dwapb-gpio.txt   | 10 -
 drivers/gpio/gpio-dwapb.c  | 44 +-
 include/linux/platform_data/gpio-dwapb.h   |  3 +-
 3 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt 
b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
index 4a75da7..e343581 100644
--- a/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
+++ b/Documentation/devicetree/bindings/gpio/snps-dwapb-gpio.txt
@@ -26,8 +26,14 @@ controller.
   the second encodes the triger flags encoded as described in
   Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - interrupt-parent : The parent interrupt controller.
-- interrupts : The interrupt to the parent controller raised when GPIOs
-  generate the interrupts.
+- interrupts : The interrupts to the parent controller raised when GPIOs
+  generate the interrupts. If the controller provides one combined interrupt
+  for all GPIOs, specify a single interrupt. If the controller provides one
+  interrupt for each GPIO, provide a list of interrupts that correspond to each
+  of the GPIO pins. When specifying multiple interrupts, if any of the GPIOs 
are
+  not connected to an interrupt, use the interrupt-mask property.
+- interrupt-mask : a 32-bit bit mask that specifies which interrupts in the 
list
+  of interrupts is valid, bit is 1 for a valid irq.
 - snps,nr-gpios : The number of pins in the port, a single cell.
 - resets : Reset line for the controller.
 
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 226977f..47d82f9 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -441,14 +441,19 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
irq_gc->chip_types[1].handler = handle_edge_irq;
 
if (!pp->irq_shared) {
-   irq_set_chained_handler_and_data(pp->irq, dwapb_irq_handler,
-gpio);
+   int i;
+
+   for (i = 0; i < pp->ngpio; i++) {
+   if (pp->irq[i])
+   irq_set_chained_handler_and_data(pp->irq[i],
+   dwapb_irq_handler, gpio);
+   }
} else {
/*
 * Request a shared IRQ since where MFD would have devices
 * using the same irq pin
 */
-   err = devm_request_irq(gpio->dev, pp->irq,
+   err = devm_request_irq(gpio->dev, pp->irq[0],
   dwapb_irq_handler_mfd,
   IRQF_SHARED, "gpio-dwapb-mfd", gpio);
if (err) {
@@ -524,7 +529,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
if (pp->idx == 0)
port->gc.set_config = dwapb_gpio_set_config;
 
-   if (pp->irq)
+   if (pp->has_irq)
dwapb_configure_irqs(gpio, port, pp);
 
err = gpiochip_add_data(>gc, port);
@@ -535,7 +540,7 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
port->is_registered = true;
 
/* Add GPIO-signaled ACPI event support */
-   if (pp->irq)
+   if (pp->has_irq)
acpi_gpiochip_request_interrupts(>gc);
 
return err;
@@ -601,13 +606,34 @@ dwapb_gpio_get_pdata(struct device *dev)
if (dev->of_node && pp->idx == 0 &&
fwnode_property_read_bool(fwnode,
  "interrupt-controller")) {
-   pp->irq = irq_of_parse_and_map(to_of_node(fwnode), 0);
-   if (!pp->irq)
+   struct device_node *np = to_of_node(fwnode);
+   u32 irq_mask = 0x;
+   int j;
+
+   /* Optional irq mask */
+   fwnode_property_read_u32(fwnode, "interrupt-mask", 
_mask);
+
+   /*
+* The IP has configuration options to allow a single
+

RE: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Phil Edworthy]

Hi Shawn,

On 28 February 2018 01:53, Shawn Lin wrote:
> On 2018/2/27 23:05, Phil Edworthy wrote:
> > On 27 February 2018 14:42, Shawn Lin wrote:
> >> On 2018/2/27 22:31, Phil Edworthy wrote:
> >>> On 27 February 2018 14:28, Shawn Lin wrote:
> >>>> 在 2018/2/27 21:55, Phil Edworthy 写道:
> >>>>> Since the controller does not support the end-of-busy IRQ, don't use
> it.
> >>>>> Otherwise, on older SD cards you will get lots of these messages:
> >>>>> "mmc0: Got data interrupt 0x0002 even though no data operation
> >>>>> was
> >>>> in progress"
> >>>>>
> >>>>
> >>>> I'm afraid you have to explain which version of arasan's IP suffer
> >>>> from this and what does the "older SD cards" mean?
> >>> Ok, I'll try to find out the IP version...
> >>> For "older SD cards", I can provide a list of a few cards that
> >>> exhibit this problem and others that don't, is that enough info?
> >>
> >> What I meant is could you elaborate more about what kind of cards,
> >> e.g, are them the  legacy SDSC cards or SDHC cards, or maybe they are
> >> only running with defaut speed? or whatever, but not just with a
> >> vague "older" cards. :)
> > Unfortunately, I have one SDHC card that works, one that doesn't. Both
> > cards are running with a 50MHz SD clock. All I know is this:
> 
> Thanks for sharing these, though it looks wired as I never remember I saw
> this problem when extensively tested SD cards on one of arasan controllers
> in 2014.
Not sure what you mean by 'wired'?

Note that this is on a relatively slow device, a dual core Cortex A7 @500MHz.
Maybe that has some effect.

It's also interesting that someone posted the same fix for Xilinx a while
back, I linked to it in the commit msg.

Thanks
Phil


> > SD cards that report unexpected interrupts:
> > 2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
> > 8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
> > 8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)
> >
> > SD cards that work ok:
> > 16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
> > 16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
> > 32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)
> >
> > Thanks
> > Phil
> >
> >>>>> This has been reported on Xilinx devices that also use the Arasan IP.
> >>>>> See https://patchwork.kernel.org/patch/8062871/
> >>>>>
> >>>>> This has been tested on the Renesas RZ/ND-DB board with the RZ/N1
> >> SoC.
> >>>>>
> >>>>> Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> >>>>> ---
> >>>>> drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
> >>>>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >>>>> b/drivers/mmc/host/sdhci-of-arasan.c
> >>>>> index c33a5f7..ab66e32 100644
> >>>>> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >>>>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >>>>> @@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data
> >>>> sdhci_arasan_pdata = {
> >>>>> .ops = _arasan_ops,
> >>>>> .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> >>>>> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> >>>>> -   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> >>>>> +   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN
> |
> >>>>> +   SDHCI_QUIRK2_STOP_WITH_TC,
> >>>>> };
> >>>>>
> >>>>> static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
> >>>>> intmask)
> >>>>>
> >>>>
> >>>>
> >>>> --
> >>>> Best Regards
> >>>> Shawn Lin
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >> --
> >> Best Regards
> >> Shawn Lin
> >
> >
> >
> >

RE: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Phil Edworthy]

Hi Shawn,

On 27 February 2018 14:42, Shawn Lin wrote:
> On 2018/2/27 22:31, Phil Edworthy wrote:
> > Hi Shawn,
> >
> > On 27 February 2018 14:28, Shawn Lin wrote:
> >> 在 2018/2/27 21:55, Phil Edworthy 写道:
> >>> Since the controller does not support the end-of-busy IRQ, don't use it.
> >>> Otherwise, on older SD cards you will get lots of these messages:
> >>> "mmc0: Got data interrupt 0x0002 even though no data operation
> >>> was
> >> in progress"
> >>>
> >>
> >> I'm afraid you have to explain which version of arasan's IP suffer
> >> from this and what does the "older SD cards" mean?
> > Ok, I'll try to find out the IP version...
> > For "older SD cards", I can provide a list of a few cards that exhibit
> > this problem and others that don't, is that enough info?
> 
> What I meant is could you elaborate more about what kind of cards, e.g, are
> them the  legacy SDSC cards or SDHC cards, or maybe they are only running
> with defaut speed? or whatever, but not just with a vague "older" cards. :)
Unfortunately, I have one SDHC card that works, one that doesn't. Both cards
are running with a 50MHz SD clock. All I know is this:

SD cards that report unexpected interrupts:
2GB Sandisk Extreme III (e624 SD02G 1.89 GiB)
8GB Sandisk (SDHC class 4)  ( SU08G 7.40 GiB)
8GB Sandisk Extreme III (SDHC class 6)(bb4e SD08G 7.61 GiB)

SD cards that work ok:
16GB Samsung (microSDHC U1 class 10)  (0001 0 14.6 GiB)
16GB Sandisk Ultra (microSDHC U1 class 10)( SL16G 14.8 GiB)
32GB Sandisk Ultra (microSDHC U1 class 10)( SL32G 29.7 GiB)

Thanks
Phil

> >>> This has been reported on Xilinx devices that also use the Arasan IP.
> >>> See https://patchwork.kernel.org/patch/8062871/
> >>>
> >>> This has been tested on the Renesas RZ/ND-DB board with the RZ/N1
> SoC.
> >>>
> >>> Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> >>> ---
> >>>drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
> >>>1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> >>> b/drivers/mmc/host/sdhci-of-arasan.c
> >>> index c33a5f7..ab66e32 100644
> >>> --- a/drivers/mmc/host/sdhci-of-arasan.c
> >>> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> >>> @@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data
> >> sdhci_arasan_pdata = {
> >>>   .ops = _arasan_ops,
> >>>   .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> >>>   .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> >>> - SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> >>> + SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> >>> + SDHCI_QUIRK2_STOP_WITH_TC,
> >>>};
> >>>
> >>>static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
> >>> intmask)
> >>>
> >>
> >>
> >> --
> >> Best Regards
> >> Shawn Lin
> >
> >
> >
> >
> 
> 
> --
> Best Regards
> Shawn Lin

RE: [PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Phil Edworthy]

Hi Adrian,

On 27 February 2018 14:08, Adrian Hunter wrote:
> On 27/02/18 15:55, Phil Edworthy wrote:
> > Since the controller does not support the end-of-busy IRQ, don't use it.
> > Otherwise, on older SD cards you will get lots of these messages:
> > "mmc0: Got data interrupt 0x0002 even though no data operation was
> in progress"
> 
> SDHCI_QUIRK2_STOP_WITH_TC may be the quirk you want but it doesn't
> match your description.  SDHCI_QUIRK2_STOP_WITH_TC is when we always
> get a TC
> (end-of-busy) IRQ with the STOP command even when we didn't ask for
> one.
> Hence the TC interrupt (0x0002) comes when we think we are already
> finished.
Right, thanks for clarifying, I'll fixup the message.

> >
> > This has been reported on Xilinx devices that also use the Arasan IP.
> > See https://patchwork.kernel.org/patch/8062871/
> >
> > This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> >  drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-arasan.c
> > b/drivers/mmc/host/sdhci-of-arasan.c
> > index c33a5f7..ab66e32 100644
> > --- a/drivers/mmc/host/sdhci-of-arasan.c
> > +++ b/drivers/mmc/host/sdhci-of-arasan.c
> > @@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data
> sdhci_arasan_pdata = {
> > .ops = _arasan_ops,
> > .quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> > -   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> > +   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
> > +   SDHCI_QUIRK2_STOP_WITH_TC,
> >  };
> >
> >  static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32
> > intmask)
> >

[PATCH] mmc: sdhci-of-arasan: Add quirk to avoid erroneous msg

[Phil Edworthy]

Since the controller does not support the end-of-busy IRQ, don't use it.
Otherwise, on older SD cards you will get lots of these messages:
"mmc0: Got data interrupt 0x0002 even though no data operation was in 
progress"

This has been reported on Xilinx devices that also use the Arasan IP.
See https://patchwork.kernel.org/patch/8062871/

This has been tested on the Renesas RZ/ND-DB board with the RZ/N1 SoC.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c 
b/drivers/mmc/host/sdhci-of-arasan.c
index c33a5f7..ab66e32 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -290,7 +290,8 @@ static const struct sdhci_pltfm_data sdhci_arasan_pdata = {
.ops = _arasan_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
-   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
+   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN |
+   SDHCI_QUIRK2_STOP_WITH_TC,
 };
 
 static u32 sdhci_arasan_cqhci_irq(struct sdhci_host *host, u32 intmask)
-- 
2.7.4

RE: [PATCH 0/2] arm: Support for Renesas RZ/N1D (R9A06G032)

[Phil Edworthy]

Hi Simon,

On 26 February 2018, Michel Pollet wrote:
> 
> This series adds the plain basic support for booting a bare
> kernel on the RZ/N1D-DB Board. It's been trimmed to the strict
> minimum as a 'base', further patches that will add the
> rest of the support, pinctrl, clock architecture and quite
> a few others.
I spoke to Magnus about helping to get the RZ/N1 patches upstream.
We're trying to sort out making the device manual and board schematics
available, hopefully it won't take too long. We're also trying to sort out
access to a board... that might take a bit longer.

btw, what do you want to do about shmobile_defconfig? Should we add
any changes needed for RZ/N1 to it? Note that only one IP block is the
same as R-Car.

Thanks
Phil

RE: [PATCH/RFC 0/4] sh: sh7722/sh7757i/sh7264/sh7269: Fix pinctrl registration

[Phil Edworthy]

Hi Geert,

Thanks for the patches, sorry I can't test these either... It's been quite a few
years since I had an sh7264 or sh7269 board!

Thanks
Phil

-Original Message-
On: 09 May 2017 16:44, Laurent Pinchart wrote:
Subject: Re: [PATCH/RFC 0/4] sh: sh7722/sh7757i/sh7264/sh7269: Fix pinctrl 
registration

Hi Geert,

Thank you for the patches.

On Tuesday 09 May 2017 16:11:53 Geert Uytterhoeven wrote:
>   Hi all,
> 
> Magnus reported that on sh7722/Migo-R, pinctrl registration fails with:
> 
> sh-pfc pfc-sh7722: pin 0 already registered
> sh-pfc pfc-sh7722: error during pin registration
> sh-pfc pfc-sh7722: could not register: -22
> sh-pfc: probe of pfc-sh7722 failed with error -22
> 
> pinmux_pins[] is initialized through PINMUX_GPIO(), using designated 
> array initializers, where the GPIO_* enums serve as indices.
> Apparently GPIO_PTQ7 was defined in the enum, but never used.
> If enum values are defined, but never used, pinmux_pins[] contains
> (zero-filled) holes.  Hence such entries are treated as pin zero, 
> which was registered before, and initialization fails.
> 
> I can't see how this ever worked, as at the time of commit 
> f5e25ae52feff2dc
> ("sh-pfc: Add sh7722 pinmux support"), pinmux_gpios[] in 
> drivers/pinctrl/sh-pfc/pfc-sh7722.c already had the hole, and 
> drivers/pinctrl/core.c already had the check.
> 
> Some scripting revealed a few more broken drivers:
>   - sh7757 has four holes, due to nonexistent GPIO_PT[JLNQ]7_RESV.
>   - sh7264 and sh7269 define GPIO_PH[0-7], but don't use it with
> PINMUX_GPIO().
> 
> Patch 1 fixes the issue on sh7722, and was tested.
> Patches 3-4 should fix the issue on the other 3 SoCs, but was untested 
> due to lack of hardware.

This all looks good to me, even if I can't test or verify patches 2/4 to 4/4 as 
I don't have access to the hardware or datasheet either. Given that they're 
untested I wouldn't fake the error message in the commit log though, but just 
refer to the problem noticed on sh7722.

Apart from that, for the whole series,

Reviewed-by: Laurent Pinchart 

> Thanks for your comments!
> 
> Geert Uytterhoeven (4):
>   sh: sh7722: Remove nonexistent GPIO_PTQ7 to fix pinctrl registration
>   [RFC] sh: sh7757: Remove nonexistent GPIO_PT[JLNQ]7_RESV to fix
> pinctrl registration
>   sh: sh7264: Remove nonexistent GPIO_PH[0-7] to fix pinctrl
> registration
>   [RFC] sh: sh7269: Remove nonexistent GPIO_PH[0-7] to fix pinctrl
> registration
> 
>  arch/sh/include/cpu-sh2a/cpu/sh7264.h | 4 +---  
> arch/sh/include/cpu-sh2a/cpu/sh7269.h | 4 +---  
> arch/sh/include/cpu-sh4/cpu/sh7722.h  | 2 +-  
> arch/sh/include/cpu-sh4/cpu/sh7757.h  | 8 
>  4 files changed, 7 insertions(+), 11 deletions(-)

--
Regards,

Laurent Pinchart

RE: [PATCH/RFC] PCI: rcar: Restrict pci_fixup_irqs to the same PCI domain

[Phil Edworthy]

Hi Simon,

On 07 November 2016 12:01, Simon Horman wrote:
> From: Phil Edworthy <phil.edwor...@renesas.com>
> 
> Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> Signed-off-by: Simon Horman <horms+rene...@verge.net.au>
> ---
> 
> Phil, this is from the 3.3.2 BSP.
> Is it appropriate for mainline?
No, this is not for upstream. The pci_fixup_irqs_local function was an RFC
that should be solved in a more generic way.
See https://patchwork.kernel.org/patch/9220653

BR
Phil

> ---
>  drivers/pci/host/pcie-rcar.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c
> index 62700d1896f4..06e7a75094c7 100644
> --- a/drivers/pci/host/pcie-rcar.c
> +++ b/drivers/pci/host/pcie-rcar.c
> @@ -472,7 +472,7 @@ static int rcar_pcie_enable(struct rcar_pcie *pcie)
>   return -ENODEV;
>   }
> 
> - pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> + pci_fixup_irqs_local(bus, pci_common_swizzle,
> of_irq_parse_and_map_pci);
> 
>   pci_bus_size_bridges(bus);
>   pci_bus_assign_resources(bus);
> --
> 2.7.0.rc3.207.g0ac5344

RE: Question about dma-ranges property for PCI host controllers

[Phil Edworthy]

Hi Arnd,

On 11 August 2016 16:09 Arnd Bergmann wrote:
> On Thursday, August 11, 2016 3:00:42 PM CEST Phil Edworthy wrote:
> > Hi,
> >
> > A few PCI host controllers use the "dma-ranges" property to specify the
> > mapping from PCI bus addresses to physical addresses.
> >
> > In the case of R-Car PCIe Host controllers, the intention was to set this
> > property as a 1 to 1 mapping for all DDR that could be addressed by the
> > device. However, there are some limitations for the R-Car controller which
> > meant that we could only map a subset of the DDR range - this limitation
> > has prompted us to work on enabling the IOMMU behind the PCI controller.
> >
> > When there is an IOMMU behind the PCI controller, the "dma-ranges"
> > property specifies the mapping from PCI bus addresses to an IOVA address.
> > So should the property map all address space?
> >
> > Note that this is not actually possible with the R-Car hardware, but I
> > found that the IOVA address space is outside of the DDR address space
> > that we were using so had change it.
> 
> It's a bit tricky: the dma-ranges properties are walked recursively,
> and a PCI bus may be behind a few other bridges that each have a
> nontrivial mapping, and the IOMMU may not be on the address space that
> the PCI host sees.
Luckily the mapping for R-Car is pretty simple, I can imagine it can get very
tricky!

> In the past, we have said that the dma-ranges property should reflect
> the address space that is used when programming the bridge registers
> in the PCI host bridge itself.
> 
> I think we have also made the assumption that a PCI host bridge
> with an IOMMU uses a flat 32-bit DMA address space that goes through
> the IOMMU (possibly a separate address space per PCI function,
> depending on the type of IOMMU).
I saw Robin Murphy's patches for PCI IOMMU map bindings, though at the
moment to get things going I'm ignoring it because it will require quite a lot
of changes to the iommu/ipmmu-vmsa driver. Other IOMMU drivers will
also have to change a fair bit to support this new binding.

> One corner case that doesn't really fit in that model is a PCI host
> bridge that requires the bridge register to be programmed in a special
> way for the IOMMU to work (e.g. away from the RAM to the address that
> is routed to the IOMMU).
In our case, there is nothing special in programming the bridge registers
for use with an IOMMU other than the range of addresses that is exposed.
The PCI host has a HW limitation that the AXI bus addresses must be
32-bit. The HW will allow you to set up the bridge registers so that PCI
bus addresses above 32-bits are mapped into the 32-bit AXI space. This
isn't used though at the moment, the PCI:AXI mapping is simply 1:1.

Simply changing the dma-ranges prop to specify all of the 32-bit range is
enough to get it to work with the IOMMU.

Without IOMMU, the dma-ranges prop was:
dma-ranges = <0x4200 0 0x4000 0 0x4000 0 0x4000>;
Note that this does not cover all of DDR, as there is 3GiB above the 32-bit
address space. Other restrictions in the way the bridge registers are
programmed mean we cannot even map all DDR in the 32-bit space.

With the IOMMU, the dma-ranges prop is simply:
dma-ranges = <0x4200 0 0x 0 0x 1 0x>;

> Another tricky case is a PCI host that uses the IOMMU only for 32-bit
> DMA masters but that does have a dma-ranges property that can be
> used for direct mapping of all RAM through a nonzero offset that
> gets set up according to dma-ranges.
I don't think that applies, though I'm struggling a bit to understand your
comment.

> Can you be more specific which of those cases you actually have here?
Hopefully I have explained it above.

Many thanks
Phil

Question about dma-ranges property for PCI host controllers

[Phil Edworthy]

Hi,

A few PCI host controllers use the "dma-ranges" property to specify the
mapping from PCI bus addresses to physical addresses.

In the case of R-Car PCIe Host controllers, the intention was to set this
property as a 1 to 1 mapping for all DDR that could be addressed by the
device. However, there are some limitations for the R-Car controller which
meant that we could only map a subset of the DDR range - this limitation
has prompted us to work on enabling the IOMMU behind the PCI controller.

When there is an IOMMU behind the PCI controller, the "dma-ranges"
property specifies the mapping from PCI bus addresses to an IOVA address.
So should the property map all address space?

Note that this is not actually possible with the R-Car hardware, but I
found that the IOVA address space is outside of the DDR address space
that we were using so had change it.

Thanks
Phil

RE: [RFC] pci: Provide a domain limited version of pdev_fixup_irq

[Phil Edworthy]

Hi Lorenzo,

Thanks for the link, I'll wait to see how this pans out.

Thanks
Phil

> -Original Message-
> From: linux-pci-ow...@vger.kernel.org [mailto:linux-pci-
> ow...@vger.kernel.org] On Behalf Of Lorenzo Pieralisi
> Sent: 21 July 2016 10:35
> To: Phil Edworthy <phil.edwor...@renesas.com>
> Cc: Bjorn Helgaas <bhelg...@google.com>; linux-...@vger.kernel.org; linux-
> renesas-...@vger.kernel.org
> Subject: Re: [RFC] pci: Provide a domain limited version of pdev_fixup_irq
> 
> Hi Phil,
> 
> On Fri, Jul 08, 2016 at 12:49:55PM +0100, Phil Edworthy wrote:
> > Hi Bjorn,
> >
> > I've marked this as RFC as I guess there might be a better way to do
> > this, but I'm not sure how. I would appreciate your thoughts on this.
> 
> There is a more generic solution to this issue, that has been
> hanging in the balance for a while:
> 
> http://marc.info/?l=linux-pci=144557674601373=2
> 
> We should bring it to completion since pci_fixup_irqs() should
> really be replaced, this is just another example of the bugs
> it can trigger.
> 
> Thanks,
> Lorenzo
> 
> > Thanks
> > Phil
> >
> > ---
> > pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> > no matter that the device may be on a completely unrelated PCI
> > Host controller.
> >
> > When you have multiple PCI Host controllers, pdev_fixup_irq() can
> > clobber the dev->irq of devices it should not be changing.
> > This has been seen when performing suspend/resume on boards with
> > two R-Car PCIe Host controllers, resulting in a NULL ptr access
> > in __pci_restore_msi_state. This happens because dev->irq has been
> > overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> >
> > This patch introduces a new function, pci_fixup_irqs_local(), that
> > performs the same operation as pdev_fixup_irq(), but only changes
> > the dev->irq of device on the same domain.
> >
> > Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> > ---
> >  drivers/pci/setup-irq.c | 25 ++---
> >  include/linux/pci.h |  3 +++
> >  2 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> > index 95c225b..90ea8fa 100644
> > --- a/drivers/pci/setup-irq.c
> > +++ b/drivers/pci/setup-irq.c
> > @@ -22,7 +22,8 @@ void __weak pcibios_update_irq(struct pci_dev *dev, int
> irq)
> > pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
> >  }
> >
> > -static void pdev_fixup_irq(struct pci_dev *dev,
> > +static void pdev_fixup_irq(int domain_nr,
> > +  struct pci_dev *dev,
> >u8 (*swizzle)(struct pci_dev *, u8 *),
> >int (*map_irq)(const struct pci_dev *, u8, u8))
> >  {
> > @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
> > if (irq == -1)
> > irq = 0;
> > }
> > -   dev->irq = irq;
> > +   /* Since pci_fixup_irqs() can be called more than once due to multiple
> > +* host controllers, and we scan all PCI devices, not just those
> > +* attached to this controller, make sure we don't clobber dev->irq
> > +* that has nothing to do with this domain.
> > +*/
> > +   if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> > +   return;
> >
> > +   dev->irq = irq;
> > dev_dbg(>dev, "fixup irq: got %d\n", dev->irq);
> >
> > /* Always tell the device, so the driver knows what is
> > @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 
> > *),
> > struct pci_dev *dev = NULL;
> >
> > for_each_pci_dev(dev)
> > -   pdev_fixup_irq(dev, swizzle, map_irq);
> > +   pdev_fixup_irq(-1, dev, swizzle, map_irq);
> >  }
> >  EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> > +
> > +void pci_fixup_irqs_local(struct pci_bus *bus,
> > +   u8 (*swizzle)(struct pci_dev *, u8 *),
> > +   int (*map_irq)(const struct pci_dev *, u8, u8))
> > +{
> > +   struct pci_dev *dev = NULL;
> > +
> > +   for_each_pci_dev(dev)
> > +   pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> > +}
> > +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 8badb66..37a97df 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1134,6 +1134,9 @@ void pdev_enable_device(

RE: [RFC] pci: Provide a domain limited version of pdev_fixup_irq

[Phil Edworthy]

Note that kbuild test failed because domain_nr is only available when we
have CONFIG_PCI_DOMAINS_GENERIC defined, so the patch will definitely
have to change.

Phil

On 08 July 2016 12:50, Phil Edworthy wrote:
> Hi Bjorn,
> 
> I've marked this as RFC as I guess there might be a better way to do
> this, but I'm not sure how. I would appreciate your thoughts on this.
> 
> Thanks
> Phil
> 
> ---
> pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
> no matter that the device may be on a completely unrelated PCI
> Host controller.
> 
> When you have multiple PCI Host controllers, pdev_fixup_irq() can
> clobber the dev->irq of devices it should not be changing.
> This has been seen when performing suspend/resume on boards with
> two R-Car PCIe Host controllers, resulting in a NULL ptr access
> in __pci_restore_msi_state. This happens because dev->irq has been
> overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.
> 
> This patch introduces a new function, pci_fixup_irqs_local(), that
> performs the same operation as pdev_fixup_irq(), but only changes
> the dev->irq of device on the same domain.
> 
> Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
> ---
>  drivers/pci/setup-irq.c | 25 ++---
>  include/linux/pci.h |  3 +++
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
> index 95c225b..90ea8fa 100644
> --- a/drivers/pci/setup-irq.c
> +++ b/drivers/pci/setup-irq.c
> @@ -22,7 +22,8 @@ void __weak pcibios_update_irq(struct pci_dev *dev, int
> irq)
>   pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
>  }
> 
> -static void pdev_fixup_irq(struct pci_dev *dev,
> +static void pdev_fixup_irq(int domain_nr,
> +struct pci_dev *dev,
>  u8 (*swizzle)(struct pci_dev *, u8 *),
>  int (*map_irq)(const struct pci_dev *, u8, u8))
>  {
> @@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
>   if (irq == -1)
>   irq = 0;
>   }
> - dev->irq = irq;
> + /* Since pci_fixup_irqs() can be called more than once due to multiple
> +  * host controllers, and we scan all PCI devices, not just those
> +  * attached to this controller, make sure we don't clobber dev->irq
> +  * that has nothing to do with this domain.
> +  */
> + if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
> + return;
> 
> + dev->irq = irq;
>   dev_dbg(>dev, "fixup irq: got %d\n", dev->irq);
> 
>   /* Always tell the device, so the driver knows what is
> @@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
>   struct pci_dev *dev = NULL;
> 
>   for_each_pci_dev(dev)
> - pdev_fixup_irq(dev, swizzle, map_irq);
> + pdev_fixup_irq(-1, dev, swizzle, map_irq);
>  }
>  EXPORT_SYMBOL_GPL(pci_fixup_irqs);
> +
> +void pci_fixup_irqs_local(struct pci_bus *bus,
> + u8 (*swizzle)(struct pci_dev *, u8 *),
> + int (*map_irq)(const struct pci_dev *, u8, u8))
> +{
> + struct pci_dev *dev = NULL;
> +
> + for_each_pci_dev(dev)
> + pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
> +}
> +EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 8badb66..37a97df 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1134,6 +1134,9 @@ void pdev_enable_device(struct pci_dev *);
>  int pci_enable_resources(struct pci_dev *, int mask);
>  void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
>   int (*)(const struct pci_dev *, u8, u8));
> +void pci_fixup_irqs_local(struct pci_bus *,
> + u8 (*)(struct pci_dev *, u8 *),
> + int (*)(const struct pci_dev *, u8, u8));
>  #define HAVE_PCI_REQ_REGIONS 2
>  int __must_check pci_request_regions(struct pci_dev *, const char *);
>  int __must_check pci_request_regions_exclusive(struct pci_dev *, const char 
> *);
> --
> 2.5.0

[RFC] pci: Provide a domain limited version of pdev_fixup_irq

[Phil Edworthy]

Hi Bjorn,

I've marked this as RFC as I guess there might be a better way to do
this, but I'm not sure how. I would appreciate your thoughts on this.

Thanks
Phil

---
pdev_fixup_irq() performs interrupt swizzling on all PCI devices,
no matter that the device may be on a completely unrelated PCI
Host controller.

When you have multiple PCI Host controllers, pdev_fixup_irq() can
clobber the dev->irq of devices it should not be changing.
This has been seen when performing suspend/resume on boards with
two R-Car PCIe Host controllers, resulting in a NULL ptr access
in __pci_restore_msi_state. This happens because dev->irq has been
overwritten with 0, and irq_get_msi_desc(dev->irq) returns NULL.

This patch introduces a new function, pci_fixup_irqs_local(), that
performs the same operation as pdev_fixup_irq(), but only changes
the dev->irq of device on the same domain.

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 drivers/pci/setup-irq.c | 25 ++---
 include/linux/pci.h |  3 +++
 2 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 95c225b..90ea8fa 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -22,7 +22,8 @@ void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
 }
 
-static void pdev_fixup_irq(struct pci_dev *dev,
+static void pdev_fixup_irq(int domain_nr,
+  struct pci_dev *dev,
   u8 (*swizzle)(struct pci_dev *, u8 *),
   int (*map_irq)(const struct pci_dev *, u8, u8))
 {
@@ -48,8 +49,15 @@ static void pdev_fixup_irq(struct pci_dev *dev,
if (irq == -1)
irq = 0;
}
-   dev->irq = irq;
+   /* Since pci_fixup_irqs() can be called more than once due to multiple
+* host controllers, and we scan all PCI devices, not just those
+* attached to this controller, make sure we don't clobber dev->irq
+* that has nothing to do with this domain.
+*/
+   if (domain_nr >= 0 && dev->bus->domain_nr != domain_nr)
+   return;
 
+   dev->irq = irq;
dev_dbg(>dev, "fixup irq: got %d\n", dev->irq);
 
/* Always tell the device, so the driver knows what is
@@ -63,6 +71,17 @@ void pci_fixup_irqs(u8 (*swizzle)(struct pci_dev *, u8 *),
struct pci_dev *dev = NULL;
 
for_each_pci_dev(dev)
-   pdev_fixup_irq(dev, swizzle, map_irq);
+   pdev_fixup_irq(-1, dev, swizzle, map_irq);
 }
 EXPORT_SYMBOL_GPL(pci_fixup_irqs);
+
+void pci_fixup_irqs_local(struct pci_bus *bus,
+   u8 (*swizzle)(struct pci_dev *, u8 *),
+   int (*map_irq)(const struct pci_dev *, u8, u8))
+{
+   struct pci_dev *dev = NULL;
+
+   for_each_pci_dev(dev)
+   pdev_fixup_irq(bus->domain_nr, dev, swizzle, map_irq);
+}
+EXPORT_SYMBOL_GPL(pci_fixup_irqs_local);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8badb66..37a97df 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1134,6 +1134,9 @@ void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
int (*)(const struct pci_dev *, u8, u8));
+void pci_fixup_irqs_local(struct pci_bus *,
+   u8 (*)(struct pci_dev *, u8 *),
+   int (*)(const struct pci_dev *, u8, u8));
 #define HAVE_PCI_REQ_REGIONS   2
 int __must_check pci_request_regions(struct pci_dev *, const char *);
 int __must_check pci_request_regions_exclusive(struct pci_dev *, const char *);
-- 
2.5.0

RE: [PATCH] ARM: dts: r8a7791: Don't disable referenced optional clocks

[Phil Edworthy]

Hi,

On 07 April 2016 20:14, Sergei Shtylyov wrote:
> On 04/07/2016 10:00 AM, Sjoerd Simons wrote:
> 
> > Hey Sergei,
> >
> > Thanks for your review.
> 
> You're welcome. :-)
> 
> > On Thu, 2016-04-07 at 02:15 +0300, Sergei Shtylyov wrote:
> >> On 04/06/2016 03:52 PM, Sjoerd Simons wrote:
> >>
> >>>
> >>> clk_get on a disabled clock node will return EPROBE_DEFER, which
> >>> can
> >>> cause drivers to be deferred forever if such clocks are referenced
> >>> in
> >>> their clocks property.
> >>>
> >>> Update the various disabled external clock nodes to default to a
> >>> frequency of 0, but don't disable them to prevent this.
> >>>
> >>> Signed-off-by: Sjoerd Simons <sjoerd.sim...@collabora.co.uk>
> >>>
> >>> ---
> >>>
> >>>arch/arm/boot/dts/r8a7791-koelsch.dts | 1 +
> >>>arch/arm/boot/dts/r8a7791-porter.dts  | 1 +
> >>>arch/arm/boot/dts/r8a7791.dtsi| 5 +
> >>>3 files changed, 3 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> index 1adf877..da59c28 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-koelsch.dts
> >>> @@ -660,6 +660,7 @@
> >>>};
> >>>
> >>>_bus_clk {
> >>> + clock-frequency = <1>;
> 
> >>  Hmmm, looking at the Koelsch schematics, I don't see this clock.
> >> :-/
> >
> > I don't have the schematics so i was simply keeping the current state.
> 
> I understand. I was just surprised by what checking the code against the
> schematics revealed.
> 
> > I've added Phil Edworthy to the list as he was the one originally
> 
> I should've CC'ed Phil myself...
>
> > enable the bus clk for Koelsh according to git. Hopefully he can
> > clarify :)
> 
> Let's hope he'd reply...
> 
> >>>   status = "okay";
> >>>};
> >>>
> >>> diff --git a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> index 9554d13..19b257e 100644
> >>> --- a/arch/arm/boot/dts/r8a7791-porter.dts
> >>> +++ b/arch/arm/boot/dts/r8a7791-porter.dts
> >>> @@ -413,6 +413,7 @@
> >>>};
> >>>
> >>>_bus_clk {
> >>> + clock-frequency = <1>;
> >>>   status = "okay";
> >>>};
> >>>
> >>  Again, looking at the Porter schematics, I don't see this clock
> >> either. :-/
> >
> > You were the one enabling this clock for Porter ;) I don't have PCIE
> 
> Yes, of course. I don't remember the gory details already -- I might have
> blindly copied what was in the Koelsch's .dts.
> 
> > hardware to test with on my porter board, might be worth if you could
> > disable this clock and see if PCI-E still fucntions as expected (maybe
> > in practise it just happens to prefer the internal clock?) ?
> 
> I know the PCIe driver does require this clock in order to function -- it
> would be the same eternal -EPROBE_DEFER condition you'd already described;
> see drivers/pci/host/pcie-rcar.c yourself...
> 
> >>> diff --git a/arch/arm/boot/dts/r8a7791.dtsi
> >>> b/arch/arm/boot/dts/r8a7791.dtsi
> >>> index 8693888..676df63 100644
> >>> --- a/arch/arm/boot/dts/r8a7791.dtsi
> >>> +++ b/arch/arm/boot/dts/r8a7791.dtsi
> >>> @@ -1104,8 +1104,7 @@
> >>>   pcie_bus_clk: pcie_bus {
> >>>   compatible = "fixed-clock";
> >>>   #clock-cells = <0>;
> >>> - clock-frequency = <1>;
> >>> - status = "disabled";
> >>> + clock-frequency = <0>;
> >>  If the clock has a good default frequency, I don't think you need
> >> to
> >> remove it. Otherwise you missed USB_EXTAL which is 48 MHz (and can be
> >> overridden).
> >
> > I did that as it was by default disabled, so if i do enable it but
> > don't drop the default frequency to 0 board swithout that clock will
> > suddenly have it added to their dtb.
> 
> Zero frequency is hardly any better then the default...
> 
> > For the usb external clock I didn't touch it as it was already enabled
> > by default with a proper frequency, so it wouldn't hit the issue i was
> > trying to fix here. But i agree, both looking at other Renesas dtsis
> > and how all other external clocks are done in this dtsi, this node is
> > odd.
> 
> It's not that odd given that the R-Car gen2 manual specifically says it
> should be 48 MHz.
> Looking into the manual again, the PCIe bus clock must be the onne
> presented on the CICREF[PN]1_SATA/PCIe input signals and it indeed should be
> 100 MHz... So Phil was correct.

PCIe always requires a 100MHz reference clock. On Koelsch, this clock is
generated by U6 and always on. Some boards may need additional hardware
to be setup in order to output the clock, hence the DT node for the clock.

I hope that clarifies the situation!
Phil

[PATCH v3 1/2] arm64: renesas: r8a7795: Add PCIe nodes

[Phil Edworthy]

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v3:
 Really use GIC_SPI for all interrupts.

v2:
 Dropped the "_clk" suffix from the PCIe bus device node's name.
 Removed "clock-output-names" property from the PCIe bus node.
 Use GIC_SPI  for all interrupts.
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 57 
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 868c10e..11f9971 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -131,6 +131,14 @@
status = "disabled";
};
 
+   /* External PCIe clock - can be overridden by the board */
+   pcie_bus_clk: pcie_bus {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <1>;
+   status = "disabled";
+   };
+
soc {
compatible = "simple-bus";
interrupt-parent = <>;
@@ -1156,5 +1164,54 @@
power-domains = <>;
status = "disabled";
};
+   pciec0: pcie@fe00 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xfe00 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xfe10 0 
0x0010
+   0x0200 0 0xfe20 0 0xfe20 0 
0x0020
+   0x0200 0 0x3000 0 0x3000 0 
0x0800
+   0x4200 0 0x3800 0 0x3800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = ,
+   ,
+   ;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  GIC_SPI 116 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 319>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
+
+   pciec1: pcie@ee80 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xee80 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xee90 0 
0x0010
+   0x0200 0 0xeea0 0 0xeea0 0 
0x0020
+   0x0200 0 0xc000 0 0xc000 0 
0x0800
+   0x4200 0 0xc800 0 0xc800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = ,
+   ,
+   ;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  GIC_SPI 148 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 318>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
};
 };
-- 
2.5.0

[PATCH v3 2/2] arm64: dts: r8a7795: enable PCIe on Salvator-X

[Phil Edworthy]

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v3: no changes
v2: no changes
---
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts 
b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index 5165d45..ccd2d7f 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -388,3 +388,15 @@
  {
status = "okay";
 };
+
+_bus_clk {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.5.0

[PATCH v2 0/2] Add PCIe to r8a7795 & Salvator-X

[Phil Edworthy]

With these two patches PCIe works on the Salvator-X board.

v2:
 Fixes for Geert's comments.

Phil Edworthy (2):
  arm64: renesas: r8a7795: Add PCIe nodes
  arm64: dts: r8a7795: enable PCIe on Salvator-X

 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 12 +
 arch/arm64/boot/dts/renesas/r8a7795.dtsi   | 57 ++
 2 files changed, 69 insertions(+)

-- 
2.5.0

[PATCH v2 2/2] arm64: dts: r8a7795: enable PCIe on Salvator-X

[Phil Edworthy]

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v2: no changes
---
 arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 12 
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts 
b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index 5165d45..ccd2d7f 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -388,3 +388,15 @@
  {
status = "okay";
 };
+
+_bus_clk {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
+
+ {
+   status = "okay";
+};
-- 
2.5.0

[PATCH v2 1/2] arm64: renesas: r8a7795: Add PCIe nodes

[Phil Edworthy]

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
v2:
 Dropped the "_clk" suffix from the PCIe bus device node's name.
 Removed "clock-output-names" property from the PCIe bus node.
 Use GIC_SPI  for all interrupts.
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 57 
 1 file changed, 57 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 868c10e..b2edc0b 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -131,6 +131,14 @@
status = "disabled";
};
 
+   /* External PCIe clock - can be overridden by the board */
+   pcie_bus_clk: pcie_bus {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <1>;
+   status = "disabled";
+   };
+
soc {
compatible = "simple-bus";
interrupt-parent = <>;
@@ -1156,5 +1164,54 @@
power-domains = <>;
status = "disabled";
};
+   pciec0: pcie@fe00 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xfe00 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xfe10 0 
0x0010
+   0x0200 0 0xfe20 0 0xfe20 0 
0x0020
+   0x0200 0 0x3000 0 0x3000 0 
0x0800
+   0x4200 0 0x3800 0 0x3800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = ,
+   ,
+   ;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  0 116 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 319>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
+
+   pciec1: pcie@ee80 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xee80 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xee90 0 
0x0010
+   0x0200 0 0xeea0 0 0xeea0 0 
0x0020
+   0x0200 0 0xc000 0 0xc000 0 
0x0800
+   0x4200 0 0xc800 0 0xc800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = ,
+   ,
+   ;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  0 148 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 318>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
};
 };
-- 
2.5.0

[PATCH 1/2] arm64: renesas: r8a7795: Add PCIe nodes

[Phil Edworthy]

Signed-off-by: Phil Edworthy <phil.edwor...@renesas.com>
---
 arch/arm64/boot/dts/renesas/r8a7795.dtsi | 58 
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi 
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 868c10e..6cfc248 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -131,6 +131,15 @@
status = "disabled";
};
 
+   /* External PCIe clock - can be overridden by the board */
+   pcie_bus_clk: pcie_bus_clk {
+   compatible = "fixed-clock";
+   #clock-cells = <0>;
+   clock-frequency = <1>;
+   clock-output-names = "pcie_bus";
+   status = "disabled";
+   };
+
soc {
compatible = "simple-bus";
interrupt-parent = <>;
@@ -1156,5 +1165,54 @@
power-domains = <>;
status = "disabled";
};
+   pciec0: pcie@fe00 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xfe00 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xfe10 0 
0x0010
+   0x0200 0 0xfe20 0 0xfe20 0 
0x0020
+   0x0200 0 0x3000 0 0x3000 0 
0x0800
+   0x4200 0 0x3800 0 0x3800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = <0 116 IRQ_TYPE_LEVEL_HIGH>,
+   <0 117 IRQ_TYPE_LEVEL_HIGH>,
+   <0 118 IRQ_TYPE_LEVEL_HIGH>;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  0 116 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 319>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
+
+   pciec1: pcie@ee80 {
+   compatible = "renesas,pcie-r8a7795";
+   reg = <0 0xee80 0 0x8>;
+   #address-cells = <3>;
+   #size-cells = <2>;
+   bus-range = <0x00 0xff>;
+   device_type = "pci";
+   ranges = <0x0100 0 0x 0 0xee90 0 
0x0010
+   0x0200 0 0xeea0 0 0xeea0 0 
0x0020
+   0x0200 0 0xc000 0 0xc000 0 
0x0800
+   0x4200 0 0xc800 0 0xc800 0 
0x0800>;
+   /* Map all possible DDR as inbound ranges */
+   dma-ranges = <0x4200 0 0x4000 0 0x4000 0 
0x4000>;
+   interrupts = <0 148 IRQ_TYPE_LEVEL_HIGH>,
+   <0 149 IRQ_TYPE_LEVEL_HIGH>,
+   <0 150 IRQ_TYPE_LEVEL_HIGH>;
+   #interrupt-cells = <1>;
+   interrupt-map-mask = <0 0 0 0>;
+   interrupt-map = <0 0 0 0  0 148 
IRQ_TYPE_LEVEL_HIGH>;
+   clocks = < CPG_MOD 318>, <_bus_clk>;
+   clock-names = "pcie", "pcie_bus";
+   power-domains = <>;
+   status = "disabled";
+   };
};
 };
-- 
2.5.0

RE: [PATCH] PCI: rcar, rcar-gen2: Use ARCH_RENESAS

[Phil Edworthy]

Hi Geert,

On 25 February 2016 08:05, Geert Uytterhoeven wrote:
> On Thu, Feb 25, 2016 at 1:45 AM, Simon Horman
>  wrote:
> > Make use of ARCH_RENESAS in place of ARCH_SHMOBILE.
> >
> > This is part of an ongoing process to migrate from ARCH_SHMOBILE to
> > ARCH_RENESAS the motivation for which being that RENESAS seems to be a
> more
> > appropriate name than SHMOBILE for the majority of Renesas ARM based
> SoCs.
> >
> > Signed-off-by: Simon Horman 
> 
> Acked-by: Geert Uytterhoeven 
> 
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> 
> > @@ -49,7 +49,7 @@ config PCI_RCAR_GEN2
> >
> >  config PCI_RCAR_GEN2_PCIE
> > bool "Renesas R-Car PCIe controller"
> > -   depends on ARCH_SHMOBILE || (ARM && COMPILE_TEST)
> > +   depends on ARCH_RENESAS || (ARM && COMPILE_TEST)
> > help
> >   Say Y here if you want PCIe controller support on R-Car Gen2 SoCs.
> 
> BTW, this looks like a misnomer: the driver supports a compatible value for
> R-Car H1, too ("renesas,pcie-r8a7779"), but it's not used in r8a7779.dtsi.
IIRC, H1 PCIe support was nobbled in some way that meant you could not
use it with any normal PCIe card. You could connect to another R-Car that
was acting as an endpoint though.

Phil