Re: [PATCH v6 1/3] fork: Add generic vmalloced stack support

[Josh Cartwright]

On Tue, Aug 30, 2016 at 03:01:51PM -0700, Andy Lutomirski wrote:
> On Wed, Aug 24, 2016 at 9:51 AM, Josh Cartwright <jo...@ni.com> wrote:
[..]
> >> diff --git a/kernel/fork.c b/kernel/fork.c
> >> index 52e725d4a866..05f7ef796fb4 100644
> >> --- a/kernel/fork.c
> >> +++ b/kernel/fork.c
> >> @@ -158,19 +158,39 @@ void __weak arch_release_thread_stack(unsigned long 
> >> *stack)
> >>   * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
> >>   * kmemcache based allocator.
> >>   */
> >> -# if THREAD_SIZE >= PAGE_SIZE
> >> -static unsigned long *alloc_thread_stack_node(struct task_struct *tsk,
> >> -   int node)
> >> +# if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
> >> +static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, 
> >> int node)
> >>  {
> >> +#ifdef CONFIG_VMAP_STACK
> >> + void *stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
> >> +VMALLOC_START, VMALLOC_END,
> >> +THREADINFO_GFP | __GFP_HIGHMEM,
> >> +PAGE_KERNEL,
> >> +0, node,
> >> +__builtin_return_address(0));
> >> +
> >> + /*
> >> +  * We can't call find_vm_area() in interrupt context, and
> >> +  * free_thread_stack can be called in interrupt context, so cache
> >> +  * the vm_struct.
> >> +  */
> >> + if (stack)
> >> + tsk->stack_vm_area = find_vm_area(stack);
> >
> > This is annoying, we end up having to walk the vm_area tree twice (once
> > for the allocation, then here to get a handle on area).
> >
> > Perhaps it's time the vmalloc code learned an allocation API that
> > returned the vm_area handle as well.
> 
> Agreed.  I may do this once everything else lands.

There are at least a few other places that could benefit from this,
doing a quick scan of find_vm_area() callers: vmalloc_{32_,}_user() and
kasan_module_alloc().

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] fork: Add generic vmalloced stack support

[Josh Cartwright]

On Tue, Aug 30, 2016 at 03:01:51PM -0700, Andy Lutomirski wrote:
> On Wed, Aug 24, 2016 at 9:51 AM, Josh Cartwright  wrote:
[..]
> >> diff --git a/kernel/fork.c b/kernel/fork.c
> >> index 52e725d4a866..05f7ef796fb4 100644
> >> --- a/kernel/fork.c
> >> +++ b/kernel/fork.c
> >> @@ -158,19 +158,39 @@ void __weak arch_release_thread_stack(unsigned long 
> >> *stack)
> >>   * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
> >>   * kmemcache based allocator.
> >>   */
> >> -# if THREAD_SIZE >= PAGE_SIZE
> >> -static unsigned long *alloc_thread_stack_node(struct task_struct *tsk,
> >> -   int node)
> >> +# if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
> >> +static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, 
> >> int node)
> >>  {
> >> +#ifdef CONFIG_VMAP_STACK
> >> + void *stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
> >> +VMALLOC_START, VMALLOC_END,
> >> +THREADINFO_GFP | __GFP_HIGHMEM,
> >> +PAGE_KERNEL,
> >> +0, node,
> >> +__builtin_return_address(0));
> >> +
> >> + /*
> >> +  * We can't call find_vm_area() in interrupt context, and
> >> +  * free_thread_stack can be called in interrupt context, so cache
> >> +  * the vm_struct.
> >> +  */
> >> + if (stack)
> >> + tsk->stack_vm_area = find_vm_area(stack);
> >
> > This is annoying, we end up having to walk the vm_area tree twice (once
> > for the allocation, then here to get a handle on area).
> >
> > Perhaps it's time the vmalloc code learned an allocation API that
> > returned the vm_area handle as well.
> 
> Agreed.  I may do this once everything else lands.

There are at least a few other places that could benefit from this,
doing a quick scan of find_vm_area() callers: vmalloc_{32_,}_user() and
kasan_module_alloc().

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] fork: Add generic vmalloced stack support

[Josh Cartwright]

Hey Andy-

Small non-critical/potential future optimization comment below:

On Thu, Aug 11, 2016 at 02:35:21AM -0700, Andy Lutomirski wrote:
> If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with
> vmalloc_node.
> 
> grsecurity has had a similar feature (called
> GRKERNSEC_KSTACKOVERFLOW) for a long time.
> 
> Cc: Oleg Nesterov 
> Signed-off-by: Andy Lutomirski 
> ---
[..]
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 52e725d4a866..05f7ef796fb4 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -158,19 +158,39 @@ void __weak arch_release_thread_stack(unsigned long 
> *stack)
>   * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
>   * kmemcache based allocator.
>   */
> -# if THREAD_SIZE >= PAGE_SIZE
> -static unsigned long *alloc_thread_stack_node(struct task_struct *tsk,
> -   int node)
> +# if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
> +static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int 
> node)
>  {
> +#ifdef CONFIG_VMAP_STACK
> + void *stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
> +VMALLOC_START, VMALLOC_END,
> +THREADINFO_GFP | __GFP_HIGHMEM,
> +PAGE_KERNEL,
> +0, node,
> +__builtin_return_address(0));
> +
> + /*
> +  * We can't call find_vm_area() in interrupt context, and
> +  * free_thread_stack can be called in interrupt context, so cache
> +  * the vm_struct.
> +  */
> + if (stack)
> + tsk->stack_vm_area = find_vm_area(stack);

This is annoying, we end up having to walk the vm_area tree twice (once
for the allocation, then here to get a handle on area).

Perhaps it's time the vmalloc code learned an allocation API that
returned the vm_area handle as well.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v6 1/3] fork: Add generic vmalloced stack support

[Josh Cartwright]

Hey Andy-

Small non-critical/potential future optimization comment below:

On Thu, Aug 11, 2016 at 02:35:21AM -0700, Andy Lutomirski wrote:
> If CONFIG_VMAP_STACK is selected, kernel stacks are allocated with
> vmalloc_node.
> 
> grsecurity has had a similar feature (called
> GRKERNSEC_KSTACKOVERFLOW) for a long time.
> 
> Cc: Oleg Nesterov 
> Signed-off-by: Andy Lutomirski 
> ---
[..]
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 52e725d4a866..05f7ef796fb4 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -158,19 +158,39 @@ void __weak arch_release_thread_stack(unsigned long 
> *stack)
>   * Allocate pages if THREAD_SIZE is >= PAGE_SIZE, otherwise use a
>   * kmemcache based allocator.
>   */
> -# if THREAD_SIZE >= PAGE_SIZE
> -static unsigned long *alloc_thread_stack_node(struct task_struct *tsk,
> -   int node)
> +# if THREAD_SIZE >= PAGE_SIZE || defined(CONFIG_VMAP_STACK)
> +static unsigned long *alloc_thread_stack_node(struct task_struct *tsk, int 
> node)
>  {
> +#ifdef CONFIG_VMAP_STACK
> + void *stack = __vmalloc_node_range(THREAD_SIZE, THREAD_SIZE,
> +VMALLOC_START, VMALLOC_END,
> +THREADINFO_GFP | __GFP_HIGHMEM,
> +PAGE_KERNEL,
> +0, node,
> +__builtin_return_address(0));
> +
> + /*
> +  * We can't call find_vm_area() in interrupt context, and
> +  * free_thread_stack can be called in interrupt context, so cache
> +  * the vm_struct.
> +  */
> + if (stack)
> + tsk->stack_vm_area = find_vm_area(stack);

This is annoying, we end up having to walk the vm_area tree twice (once
for the allocation, then here to get a handle on area).

Perhaps it's time the vmalloc code learned an allocation API that
returned the vm_area handle as well.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Mon, May 02, 2016 at 12:08:50PM -0700, Florian Fainelli wrote:
> On 02/05/16 11:36, Josh Cartwright wrote:
> > On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
> > [..]
> >>>  static int macb_mii_init(struct macb *bp)
> >>>  {
> >>>   struct macb_platform_data *pdata;
> >>>   struct device_node *np;
> >>> - int err = -ENXIO, i;
> >>> + int err = -ENXIO;
> >>>  
> >>>   /* Enable management port */
> >>>   macb_writel(bp, NCR, MACB_BIT(MPE));
> >>> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> >>>   dev_set_drvdata(>dev->dev, bp->mii_bus);
> >>>  
> >>>   np = bp->pdev->dev.of_node;
> >>> - if (np) {
> >>> - /* try dt phy registration */
> >>> - err = of_mdiobus_register(bp->mii_bus, np);
> >>> -
> >>> - /* fallback to standard phy registration if no phy were
> >>> -  * found during dt phy registration
> >>> -  */
> >>> - if (!err && !phy_find_first(bp->mii_bus)) {
> >>> - for (i = 0; i < PHY_MAX_ADDR; i++) {
> >>> - struct phy_device *phydev;
> >>> -
> >>> - phydev = mdiobus_scan(bp->mii_bus, i);
> >>> - if (IS_ERR(phydev)) {
> >>> - err = PTR_ERR(phydev);
> >>> - break;
> >>> - }
> >>> - }
> >>> -
> >>> - if (err)
> >>> - goto err_out_unregister_bus;
> >>> - }
> >>> - } else {
> >>> - if (pdata)
> >>> - bp->mii_bus->phy_mask = pdata->phy_mask;
> >>> -
> >>> - err = mdiobus_register(bp->mii_bus);
> >>> - }
> >>> + if (np)
> >>> + err = macb_mii_of_init(bp, np);
> >>> + else
> >>> + err = macb_mii_pdata_init(bp, pdata);
> >>>  
> >>>   if (err)
> >>>   goto err_out_free_mdiobus;
> >>
> >> I'm okay with this. Thanks for having taken the initiative to implement it.
> > 
> > Unfortunately, I don't think it's going to be as straightforward
> > as I originally thought.  Still doable, but more complicated.
> > 
> > In particular, the macb bindings allow for a user to specify a
> > 'reset-gpios' property _at the PHY_ level, which is consumed by the
> > macb to adjust the PHY reset state on remove.
> 
> In fact, not just on remove, anytime there is an opportunity to save
> power (interface down, closed) and putting the PHY into reset is usually
> guaranteed to be saving more power than e.g: a BMCR power down.

I can understand how that might have been a long term goal of managing a
reset GPIO in general, however as it stands in the macb driver the only
callsite where the reset gpio is tweaked macb_remove().

> > My question is: why is the PHY reset GPIO management not the
> > responsibility of the PHY driver/core itself?
> 
> Well, this is actually being worked on at the moment by Sergei, since
> there is not necessarily a reason why PHYLIB can't deal with that:
> 
> https://lkml.org/lkml/2016/4/28/831

Cool, thanks.  I was about to see about implementing this...but since
it's already been done, I'll rebase my set on Sergei's changes.

Thanks,
  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Mon, May 02, 2016 at 12:08:50PM -0700, Florian Fainelli wrote:
> On 02/05/16 11:36, Josh Cartwright wrote:
> > On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
> > [..]
> >>>  static int macb_mii_init(struct macb *bp)
> >>>  {
> >>>   struct macb_platform_data *pdata;
> >>>   struct device_node *np;
> >>> - int err = -ENXIO, i;
> >>> + int err = -ENXIO;
> >>>  
> >>>   /* Enable management port */
> >>>   macb_writel(bp, NCR, MACB_BIT(MPE));
> >>> @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> >>>   dev_set_drvdata(>dev->dev, bp->mii_bus);
> >>>  
> >>>   np = bp->pdev->dev.of_node;
> >>> - if (np) {
> >>> - /* try dt phy registration */
> >>> - err = of_mdiobus_register(bp->mii_bus, np);
> >>> -
> >>> - /* fallback to standard phy registration if no phy were
> >>> -  * found during dt phy registration
> >>> -  */
> >>> - if (!err && !phy_find_first(bp->mii_bus)) {
> >>> - for (i = 0; i < PHY_MAX_ADDR; i++) {
> >>> - struct phy_device *phydev;
> >>> -
> >>> - phydev = mdiobus_scan(bp->mii_bus, i);
> >>> - if (IS_ERR(phydev)) {
> >>> - err = PTR_ERR(phydev);
> >>> - break;
> >>> - }
> >>> - }
> >>> -
> >>> - if (err)
> >>> - goto err_out_unregister_bus;
> >>> - }
> >>> - } else {
> >>> - if (pdata)
> >>> - bp->mii_bus->phy_mask = pdata->phy_mask;
> >>> -
> >>> - err = mdiobus_register(bp->mii_bus);
> >>> - }
> >>> + if (np)
> >>> + err = macb_mii_of_init(bp, np);
> >>> + else
> >>> + err = macb_mii_pdata_init(bp, pdata);
> >>>  
> >>>   if (err)
> >>>   goto err_out_free_mdiobus;
> >>
> >> I'm okay with this. Thanks for having taken the initiative to implement it.
> > 
> > Unfortunately, I don't think it's going to be as straightforward
> > as I originally thought.  Still doable, but more complicated.
> > 
> > In particular, the macb bindings allow for a user to specify a
> > 'reset-gpios' property _at the PHY_ level, which is consumed by the
> > macb to adjust the PHY reset state on remove.
> 
> In fact, not just on remove, anytime there is an opportunity to save
> power (interface down, closed) and putting the PHY into reset is usually
> guaranteed to be saving more power than e.g: a BMCR power down.

I can understand how that might have been a long term goal of managing a
reset GPIO in general, however as it stands in the macb driver the only
callsite where the reset gpio is tweaked macb_remove().

> > My question is: why is the PHY reset GPIO management not the
> > responsibility of the PHY driver/core itself?
> 
> Well, this is actually being worked on at the moment by Sergei, since
> there is not necessarily a reason why PHYLIB can't deal with that:
> 
> https://lkml.org/lkml/2016/4/28/831

Cool, thanks.  I was about to see about implementing this...but since
it's already been done, I'll rebase my set on Sergei's changes.

Thanks,
  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
[..]
> >  static int macb_mii_init(struct macb *bp)
> >  {
> > struct macb_platform_data *pdata;
> > struct device_node *np;
> > -   int err = -ENXIO, i;
> > +   int err = -ENXIO;
> >  
> > /* Enable management port */
> > macb_writel(bp, NCR, MACB_BIT(MPE));
> > @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> > dev_set_drvdata(>dev->dev, bp->mii_bus);
> >  
> > np = bp->pdev->dev.of_node;
> > -   if (np) {
> > -   /* try dt phy registration */
> > -   err = of_mdiobus_register(bp->mii_bus, np);
> > -
> > -   /* fallback to standard phy registration if no phy were
> > -* found during dt phy registration
> > -*/
> > -   if (!err && !phy_find_first(bp->mii_bus)) {
> > -   for (i = 0; i < PHY_MAX_ADDR; i++) {
> > -   struct phy_device *phydev;
> > -
> > -   phydev = mdiobus_scan(bp->mii_bus, i);
> > -   if (IS_ERR(phydev)) {
> > -   err = PTR_ERR(phydev);
> > -   break;
> > -   }
> > -   }
> > -
> > -   if (err)
> > -   goto err_out_unregister_bus;
> > -   }
> > -   } else {
> > -   if (pdata)
> > -   bp->mii_bus->phy_mask = pdata->phy_mask;
> > -
> > -   err = mdiobus_register(bp->mii_bus);
> > -   }
> > +   if (np)
> > +   err = macb_mii_of_init(bp, np);
> > +   else
> > +   err = macb_mii_pdata_init(bp, pdata);
> >  
> > if (err)
> > goto err_out_free_mdiobus;
> 
> I'm okay with this. Thanks for having taken the initiative to implement it.

Unfortunately, I don't think it's going to be as straightforward
as I originally thought.  Still doable, but more complicated.

In particular, the macb bindings allow for a user to specify a
'reset-gpios' property _at the PHY_ level, which is consumed by the
macb to adjust the PHY reset state on remove.

My question is: why is the PHY reset GPIO management not the
responsibility of the PHY driver/core itself?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Fri, Apr 29, 2016 at 02:40:53PM +0200, Nicolas Ferre wrote:
[..]
> >  static int macb_mii_init(struct macb *bp)
> >  {
> > struct macb_platform_data *pdata;
> > struct device_node *np;
> > -   int err = -ENXIO, i;
> > +   int err = -ENXIO;
> >  
> > /* Enable management port */
> > macb_writel(bp, NCR, MACB_BIT(MPE));
> > @@ -446,33 +497,10 @@ static int macb_mii_init(struct macb *bp)
> > dev_set_drvdata(>dev->dev, bp->mii_bus);
> >  
> > np = bp->pdev->dev.of_node;
> > -   if (np) {
> > -   /* try dt phy registration */
> > -   err = of_mdiobus_register(bp->mii_bus, np);
> > -
> > -   /* fallback to standard phy registration if no phy were
> > -* found during dt phy registration
> > -*/
> > -   if (!err && !phy_find_first(bp->mii_bus)) {
> > -   for (i = 0; i < PHY_MAX_ADDR; i++) {
> > -   struct phy_device *phydev;
> > -
> > -   phydev = mdiobus_scan(bp->mii_bus, i);
> > -   if (IS_ERR(phydev)) {
> > -   err = PTR_ERR(phydev);
> > -   break;
> > -   }
> > -   }
> > -
> > -   if (err)
> > -   goto err_out_unregister_bus;
> > -   }
> > -   } else {
> > -   if (pdata)
> > -   bp->mii_bus->phy_mask = pdata->phy_mask;
> > -
> > -   err = mdiobus_register(bp->mii_bus);
> > -   }
> > +   if (np)
> > +   err = macb_mii_of_init(bp, np);
> > +   else
> > +   err = macb_mii_pdata_init(bp, pdata);
> >  
> > if (err)
> > goto err_out_free_mdiobus;
> 
> I'm okay with this. Thanks for having taken the initiative to implement it.

Unfortunately, I don't think it's going to be as straightforward
as I originally thought.  Still doable, but more complicated.

In particular, the macb bindings allow for a user to specify a
'reset-gpios' property _at the PHY_ level, which is consumed by the
macb to adjust the PHY reset state on remove.

My question is: why is the PHY reset GPIO management not the
responsibility of the PHY driver/core itself?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 07:34:59PM -0500, Josh Cartwright wrote:
> On Thu, Apr 28, 2016 at 11:23:15PM +0200, Andrew Lunn wrote:
> > On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote:
> > > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> > > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > > > > problem, since
> > > > > > > we have no children on the second ethernet MAC in our devices' 
> > > > > > > device trees. I'm
> > > > > > > starting to feel like our second MAC shouldn't even really 
> > > > > > > register the MDIO bus
> > > > > > > since it isn't being used - maybe adding a DT property to not 
> > > > > > > have a bus is a
> > > > > > > better option?
> > > > > > 
> > > > > > status = "disabled"
> > > > > > 
> > > > > > would be the unusual way.
> > > > > > 
> > > > > >   Andrew
> > > > > 
> > > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on 
> > > > > the MDIO
> > > > > bus of the first MAC.  So, the second MAC is used for ethernet but 
> > > > > not for MDIO,
> > > > > and so it does not have any PHYs under its DT node.  It would be nice 
> > > > > if there
> > > > > were a way to tell macb not to bother with MDIO for the second MAC, 
> > > > > since that's
> > > > > handled by the first MAC.
> > > > 
> > > > Yes, exactly, add support for status = "disabled" in the mdio node.
> > > 
> > > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
> > > the node representing the mdio bus is the same node which represents the
> > > macb instance itself.  Setting 'status = "disabled"' on this node will
> > > just prevent the probing of the macb instance.
> > 
> > :-(
> > 
> > It is very common to have an mdio node within the MAC node, for example 
> > imx6sx-sdb.dtsi
> 
> Okay, I think that makes sense.  I think, then, perhaps the solution to
> our problem is to:
> 
>   1. Modify the macb driver to support an 'mdio' node. (And adjust the
>  binding document accordingly).  If the node is found, it's used for
>  of_mdiobus_register() w/o any of the manual scan madness.
>   2. For backwards compatibility, in the case where an 'mdio' node does
>  not exist, leave the existing behavior the way it is now
>  (of_mdiobus_register() followed by manual scan) [perhaps warn of
>  deprecation as well?]
>   3. Update binding docs to reflect the above.
> 
> In this way, for our usecase, the 'status = "disabled"' in the newly
> created 'mdio' node isn't necessary.  It's sufficient for the node to
> exist and be empty.

Here's a (only build tested) attempt at implementing a part of this.  I
macb_mii_init() was getting complicated enough that I lifted out two
helper functions for the dt/no-dt case.  Sweeping the in-tree
devicetrees to update them to place phys under an 'mdio' node is still
to be done.

  Josh

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index eec3200..d843bc9 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -419,11 +419,62 @@ static int macb_mii_probe(struct net_device *dev)
return 0;
 }
 
+static int macb_mii_of_init(struct macb *bp, struct device_node *np)
+{
+   struct device_node *mdio;
+   int err, i;
+
+   mdio = of_get_child_by_name(np, "mdio");
+   if (mdio)
+   return of_mdiobus_register(bp->mii_bus, mdio);
+
+   dev_warn(>pdev->dev,
+"using deprecated PHY probing mechanism.  Please update device 
tree.");
+
+   /* try dt phy registration */
+   err = of_mdiobus_register(bp->mii_bus, np);
+   if (err)
+   return err;
+
+   /* fallback to standard phy registration if no phy were
+* found during dt phy registration
+*/
+   if (!phy_find_first(bp->mii_bus)) {
+   for (i = 0; i < PHY_MAX_ADDR; i++) {
+   struct phy_device *phydev;
+
+   phydev = mdiobus_scan(bp->mii_bus, i);
+   if (IS_ERR(phydev)) {
+   err = PTR_ERR(phydev)

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 07:34:59PM -0500, Josh Cartwright wrote:
> On Thu, Apr 28, 2016 at 11:23:15PM +0200, Andrew Lunn wrote:
> > On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote:
> > > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> > > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > > > > problem, since
> > > > > > > we have no children on the second ethernet MAC in our devices' 
> > > > > > > device trees. I'm
> > > > > > > starting to feel like our second MAC shouldn't even really 
> > > > > > > register the MDIO bus
> > > > > > > since it isn't being used - maybe adding a DT property to not 
> > > > > > > have a bus is a
> > > > > > > better option?
> > > > > > 
> > > > > > status = "disabled"
> > > > > > 
> > > > > > would be the unusual way.
> > > > > > 
> > > > > >   Andrew
> > > > > 
> > > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on 
> > > > > the MDIO
> > > > > bus of the first MAC.  So, the second MAC is used for ethernet but 
> > > > > not for MDIO,
> > > > > and so it does not have any PHYs under its DT node.  It would be nice 
> > > > > if there
> > > > > were a way to tell macb not to bother with MDIO for the second MAC, 
> > > > > since that's
> > > > > handled by the first MAC.
> > > > 
> > > > Yes, exactly, add support for status = "disabled" in the mdio node.
> > > 
> > > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
> > > the node representing the mdio bus is the same node which represents the
> > > macb instance itself.  Setting 'status = "disabled"' on this node will
> > > just prevent the probing of the macb instance.
> > 
> > :-(
> > 
> > It is very common to have an mdio node within the MAC node, for example 
> > imx6sx-sdb.dtsi
> 
> Okay, I think that makes sense.  I think, then, perhaps the solution to
> our problem is to:
> 
>   1. Modify the macb driver to support an 'mdio' node. (And adjust the
>  binding document accordingly).  If the node is found, it's used for
>  of_mdiobus_register() w/o any of the manual scan madness.
>   2. For backwards compatibility, in the case where an 'mdio' node does
>  not exist, leave the existing behavior the way it is now
>  (of_mdiobus_register() followed by manual scan) [perhaps warn of
>  deprecation as well?]
>   3. Update binding docs to reflect the above.
> 
> In this way, for our usecase, the 'status = "disabled"' in the newly
> created 'mdio' node isn't necessary.  It's sufficient for the node to
> exist and be empty.

Here's a (only build tested) attempt at implementing a part of this.  I
macb_mii_init() was getting complicated enough that I lifted out two
helper functions for the dt/no-dt case.  Sweeping the in-tree
devicetrees to update them to place phys under an 'mdio' node is still
to be done.

  Josh

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index eec3200..d843bc9 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -419,11 +419,62 @@ static int macb_mii_probe(struct net_device *dev)
return 0;
 }
 
+static int macb_mii_of_init(struct macb *bp, struct device_node *np)
+{
+   struct device_node *mdio;
+   int err, i;
+
+   mdio = of_get_child_by_name(np, "mdio");
+   if (mdio)
+   return of_mdiobus_register(bp->mii_bus, mdio);
+
+   dev_warn(>pdev->dev,
+"using deprecated PHY probing mechanism.  Please update device 
tree.");
+
+   /* try dt phy registration */
+   err = of_mdiobus_register(bp->mii_bus, np);
+   if (err)
+   return err;
+
+   /* fallback to standard phy registration if no phy were
+* found during dt phy registration
+*/
+   if (!phy_find_first(bp->mii_bus)) {
+   for (i = 0; i < PHY_MAX_ADDR; i++) {
+   struct phy_device *phydev;
+
+   phydev = mdiobus_scan(bp->mii_bus, i);
+   if (IS_ERR(phydev)) {
+   err = PTR_ERR(phydev)

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 11:23:15PM +0200, Andrew Lunn wrote:
> On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote:
> > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > > > problem, since
> > > > > > we have no children on the second ethernet MAC in our devices' 
> > > > > > device trees. I'm
> > > > > > starting to feel like our second MAC shouldn't even really register 
> > > > > > the MDIO bus
> > > > > > since it isn't being used - maybe adding a DT property to not have 
> > > > > > a bus is a
> > > > > > better option?
> > > > > 
> > > > > status = "disabled"
> > > > > 
> > > > > would be the unusual way.
> > > > > 
> > > > >   Andrew
> > > > 
> > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on 
> > > > the MDIO
> > > > bus of the first MAC.  So, the second MAC is used for ethernet but not 
> > > > for MDIO,
> > > > and so it does not have any PHYs under its DT node.  It would be nice 
> > > > if there
> > > > were a way to tell macb not to bother with MDIO for the second MAC, 
> > > > since that's
> > > > handled by the first MAC.
> > > 
> > > Yes, exactly, add support for status = "disabled" in the mdio node.
> > 
> > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
> > the node representing the mdio bus is the same node which represents the
> > macb instance itself.  Setting 'status = "disabled"' on this node will
> > just prevent the probing of the macb instance.
> 
> :-(
> 
> It is very common to have an mdio node within the MAC node, for example 
> imx6sx-sdb.dtsi

Okay, I think that makes sense.  I think, then, perhaps the solution to
our problem is to:

  1. Modify the macb driver to support an 'mdio' node. (And adjust the
 binding document accordingly).  If the node is found, it's used for
 of_mdiobus_register() w/o any of the manual scan madness.
  2. For backwards compatibility, in the case where an 'mdio' node does
 not exist, leave the existing behavior the way it is now
 (of_mdiobus_register() followed by manual scan) [perhaps warn of
 deprecation as well?]
  3. Update binding docs to reflect the above.

In this way, for our usecase, the 'status = "disabled"' in the newly
created 'mdio' node isn't necessary.  It's sufficient for the node to
exist and be empty.

>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet1>;
> phy-supply = <_enet_3v3>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> 
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> 
> ethphy1: ethernet-phy@1 {
> reg = <1>;
> };
> 
> ethphy2: ethernet-phy@2 {
> reg = <2>;
> };
> };
> };
> 
>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet2>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> };
> 
> This even has the two phys on one bus, as you described...

Yep...looks nearly exactly the same case.

Thanks,
  Josh

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 11:23:15PM +0200, Andrew Lunn wrote:
> On Thu, Apr 28, 2016 at 04:03:57PM -0500, Josh Cartwright wrote:
> > On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> > > On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > > > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > > > problem, since
> > > > > > we have no children on the second ethernet MAC in our devices' 
> > > > > > device trees. I'm
> > > > > > starting to feel like our second MAC shouldn't even really register 
> > > > > > the MDIO bus
> > > > > > since it isn't being used - maybe adding a DT property to not have 
> > > > > > a bus is a
> > > > > > better option?
> > > > > 
> > > > > status = "disabled"
> > > > > 
> > > > > would be the unusual way.
> > > > > 
> > > > >   Andrew
> > > > 
> > > > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on 
> > > > the MDIO
> > > > bus of the first MAC.  So, the second MAC is used for ethernet but not 
> > > > for MDIO,
> > > > and so it does not have any PHYs under its DT node.  It would be nice 
> > > > if there
> > > > were a way to tell macb not to bother with MDIO for the second MAC, 
> > > > since that's
> > > > handled by the first MAC.
> > > 
> > > Yes, exactly, add support for status = "disabled" in the mdio node.
> > 
> > Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
> > the node representing the mdio bus is the same node which represents the
> > macb instance itself.  Setting 'status = "disabled"' on this node will
> > just prevent the probing of the macb instance.
> 
> :-(
> 
> It is very common to have an mdio node within the MAC node, for example 
> imx6sx-sdb.dtsi

Okay, I think that makes sense.  I think, then, perhaps the solution to
our problem is to:

  1. Modify the macb driver to support an 'mdio' node. (And adjust the
 binding document accordingly).  If the node is found, it's used for
 of_mdiobus_register() w/o any of the manual scan madness.
  2. For backwards compatibility, in the case where an 'mdio' node does
 not exist, leave the existing behavior the way it is now
 (of_mdiobus_register() followed by manual scan) [perhaps warn of
 deprecation as well?]
  3. Update binding docs to reflect the above.

In this way, for our usecase, the 'status = "disabled"' in the newly
created 'mdio' node isn't necessary.  It's sufficient for the node to
exist and be empty.

>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet1>;
> phy-supply = <_enet_3v3>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> 
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> 
> ethphy1: ethernet-phy@1 {
> reg = <1>;
> };
> 
> ethphy2: ethernet-phy@2 {
> reg = <2>;
> };
> };
> };
> 
>  {
> pinctrl-names = "default";
> pinctrl-0 = <_enet2>;
> phy-mode = "rgmii";
> phy-handle = <>;
> status = "okay";
> };
> 
> This even has the two phys on one bus, as you described...

Yep...looks nearly exactly the same case.

Thanks,
  Josh

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > problem, since
> > > > we have no children on the second ethernet MAC in our devices' device 
> > > > trees. I'm
> > > > starting to feel like our second MAC shouldn't even really register the 
> > > > MDIO bus
> > > > since it isn't being used - maybe adding a DT property to not have a 
> > > > bus is a
> > > > better option?
> > > 
> > > status = "disabled"
> > > 
> > > would be the unusual way.
> > > 
> > >   Andrew
> > 
> > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the 
> > MDIO
> > bus of the first MAC.  So, the second MAC is used for ethernet but not for 
> > MDIO,
> > and so it does not have any PHYs under its DT node.  It would be nice if 
> > there
> > were a way to tell macb not to bother with MDIO for the second MAC, since 
> > that's
> > handled by the first MAC.
> 
> Yes, exactly, add support for status = "disabled" in the mdio node.

Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
the node representing the mdio bus is the same node which represents the
macb instance itself.  Setting 'status = "disabled"' on this node will
just prevent the probing of the macb instance.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 08:59:32PM +0200, Andrew Lunn wrote:
> On Thu, Apr 28, 2016 at 01:55:27PM -0500, Nathan Sullivan wrote:
> > On Thu, Apr 28, 2016 at 08:43:03PM +0200, Andrew Lunn wrote:
> > > > I agree that is a valid fix for AT91, however it won't solve our 
> > > > problem, since
> > > > we have no children on the second ethernet MAC in our devices' device 
> > > > trees. I'm
> > > > starting to feel like our second MAC shouldn't even really register the 
> > > > MDIO bus
> > > > since it isn't being used - maybe adding a DT property to not have a 
> > > > bus is a
> > > > better option?
> > > 
> > > status = "disabled"
> > > 
> > > would be the unusual way.
> > > 
> > >   Andrew
> > 
> > Oh, sorry, I meant we use both MACs on Zynq, however the PHYs are on the 
> > MDIO
> > bus of the first MAC.  So, the second MAC is used for ethernet but not for 
> > MDIO,
> > and so it does not have any PHYs under its DT node.  It would be nice if 
> > there
> > were a way to tell macb not to bother with MDIO for the second MAC, since 
> > that's
> > handled by the first MAC.
> 
> Yes, exactly, add support for status = "disabled" in the mdio node.

Unfortunately, the 'macb' doesn't have a "mdio node", or alternatively:
the node representing the mdio bus is the same node which represents the
macb instance itself.  Setting 'status = "disabled"' on this node will
just prevent the probing of the macb instance.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 09:19:47AM -0500, Nathan Sullivan wrote:
> Since of_mdiobus_register and mdiobus_register will scan automatically,

This is only partially true.  of_mdiobus_register() only scans for PHYs
with device tree presence (starting with nodes which specify an address,
then continuing for nodes without an address specifier).

So, this patch will regress any platform currently relying on a PHY
being found/probed even though it has no representation as a device tree
node.  I don't have enough history with this driver to make a judgement
as to whether or not there are any users of this functionality.

> do not manually scan for PHY devices in the macb ethernet driver. Doing
> so will result in many nonexistent PHYs on the MDIO bus if the MDIO
> lines are floating or grounded, such as when they are not used.

Just a restatement/elaboration of the problem in an attempt to provide
more context:

The assumption being made is that the phy associated with a given macb
instance exists on that instance's controlled mdio bus.  This assumption
doesn't hold true on some dual-ethernet Zynq 7000-series based designs
where both phys exist on the same MDIO bus (for pinning reasons, or
whatever).

 MDIO
macb0 +- phy0, addrX
  |
  +- phy1, addrY

macb1 - {MDIO lines NC, not pinmuxed}

(In this case, the phy associated with macb1 is phy1, which exists on
the MDIO bus controlled by the macb0 instance).

HTH,

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] net: macb: do not scan PHYs manually

[Josh Cartwright]

On Thu, Apr 28, 2016 at 09:19:47AM -0500, Nathan Sullivan wrote:
> Since of_mdiobus_register and mdiobus_register will scan automatically,

This is only partially true.  of_mdiobus_register() only scans for PHYs
with device tree presence (starting with nodes which specify an address,
then continuing for nodes without an address specifier).

So, this patch will regress any platform currently relying on a PHY
being found/probed even though it has no representation as a device tree
node.  I don't have enough history with this driver to make a judgement
as to whether or not there are any users of this functionality.

> do not manually scan for PHY devices in the macb ethernet driver. Doing
> so will result in many nonexistent PHYs on the MDIO bus if the MDIO
> lines are floating or grounded, such as when they are not used.

Just a restatement/elaboration of the problem in an attempt to provide
more context:

The assumption being made is that the phy associated with a given macb
instance exists on that instance's controlled mdio bus.  This assumption
doesn't hold true on some dual-ethernet Zynq 7000-series based designs
where both phys exist on the same MDIO bus (for pinning reasons, or
whatever).

 MDIO
macb0 +- phy0, addrX
  |
  +- phy1, addrY

macb1 - {MDIO lines NC, not pinmuxed}

(In this case, the phy associated with macb1 is phy1, which exists on
the MDIO bus controlled by the macb0 instance).

HTH,

  Josh


signature.asc
Description: PGP signature

Re: Issue with ioremap

[Josh Cartwright]

On Fri, Apr 01, 2016 at 01:13:06AM +0530, punnaiah choudary kalluri wrote:
> Hi,
> 
> We are using the pl353 smc controller for interfacing the nand in our zynq 
> SOC.
> The driver for this controller is currently under mainline review.
> Recently we are moved to 4.4 kernel and observing issues with the driver.
> while debug, found that the issue is with the virtual address returned from
> the ioremap is not aligned to the physical address and causing nand
> access failures.
> the nand controller physical address starts at 0xE100 and the size is 
> 16MB.
> the ioremap function in 4.3 kernel returns the virtual address that is
> aligned to the size
> but not the case in 4.4 kernel.

:(.  I had actually ran into this, too, as I was evaluating the use of
the upstream-targetted pl353 stuff; sorry I didn't say anything.

> this controller uses the bits [31:24] as base address and use rest all
> bits for configuring adders cycles, chip select information. so it
> expects the virtual address also aligned to 0xFF00 otherwise the
> nand commands issued will fail.

The driver _currently_ expects the virtual address to be 16M aligned,
but is that a hard requirement?  It seems possible that the driver could
be written without this assumption, correct?

This would mean that the driver would need to maintain the cs/cycles
configuration state outside of the mapped virtual address, and then
calculate + add the calculated offset to the base.  Would that work?
I had been meaning to give it a try, but haven't gotten around to it.

  Josh

Re: Issue with ioremap

[Josh Cartwright]

On Fri, Apr 01, 2016 at 01:13:06AM +0530, punnaiah choudary kalluri wrote:
> Hi,
> 
> We are using the pl353 smc controller for interfacing the nand in our zynq 
> SOC.
> The driver for this controller is currently under mainline review.
> Recently we are moved to 4.4 kernel and observing issues with the driver.
> while debug, found that the issue is with the virtual address returned from
> the ioremap is not aligned to the physical address and causing nand
> access failures.
> the nand controller physical address starts at 0xE100 and the size is 
> 16MB.
> the ioremap function in 4.3 kernel returns the virtual address that is
> aligned to the size
> but not the case in 4.4 kernel.

:(.  I had actually ran into this, too, as I was evaluating the use of
the upstream-targetted pl353 stuff; sorry I didn't say anything.

> this controller uses the bits [31:24] as base address and use rest all
> bits for configuring adders cycles, chip select information. so it
> expects the virtual address also aligned to 0xFF00 otherwise the
> nand commands issued will fail.

The driver _currently_ expects the virtual address to be 16M aligned,
but is that a hard requirement?  It seems possible that the driver could
be written without this assumption, correct?

This would mean that the driver would need to maintain the cs/cycles
configuration state outside of the mapped virtual address, and then
calculate + add the calculated offset to the base.  Would that work?
I had been meaning to give it a try, but haven't gotten around to it.

  Josh

Re: [ANNOUNCE] 3.14.64-rt67

[Josh Cartwright]

On Tue, Mar 15, 2016 at 10:50:31PM -0400, Paul Gortmaker wrote:
> On Tue, Mar 15, 2016 at 7:25 PM, Paul Gortmaker
>  wrote:
> > On Tue, Mar 15, 2016 at 5:45 PM, Paul Gortmaker
> >  wrote:
> >> On Mon, Mar 14, 2016 at 11:49 AM, Steven Rostedt  
> >> wrote:
> >>>
> >>> Dear RT Folks,
> >>>
> >>> 3.14 release on PI(E) Day!
> >>>
> >>> I'm pleased to announce the 3.14.64-rt67 stable release.
> >>
> >> Testing this with what is largely a x86-64 defconfig but with RT_FULL,
> >> I now see:
> >>
> >> root@dell760-paul:~# dmesg|grep NOH
> >> [8.605854] NOHZ: local_softirq_pending 100
> >> [8.732677] NOHZ: local_softirq_pending 100
> >> [8.852729] NOHZ: local_softirq_pending 100
> >> [8.963964] NOHZ: local_softirq_pending 100
> >> [9.061892] NOHZ: local_softirq_pending 100
> >> [9.184921] NOHZ: local_softirq_pending 100
> >> [9.370958] NOHZ: local_softirq_pending 100
> >> [9.657811] NOHZ: local_softirq_pending 100
> >> [9.942631] NOHZ: local_softirq_pending 100
> >> [   10.783710] NOHZ: local_softirq_pending 100
> >> root@dell760-paul:~#
> >>
> >> ...early in boot (we cap them after ~10 msgs).
> >>
> >> I think 100 is RCU if I did my bit counting properly; remind
> >> me to submit a patch that uses the human readable names.

As mentioned on IRC, 100 is HRTIMER_SOFTIRQ, which I think makes more
sense...at least it meshes better with the commit you identified as
being problematic.

> >>
> >> I had a good hunch which commit was responsible but I did
> >> a check of it and the one directly underneath it to be sure,
> >> and the latter boots w/o any pending messages.
> >>
> >> git log --oneline  v3.14-rt ^v3.14.64
> >> [...]
> >> 0a80a6849f19 latencyhist: disable jump-labels
> >> a884ef48e1ca net: provide a way to delegate processing a softirq to 
> >> ksoftirqd
> >> 780d7ca2fdb0 softirq: split timer softirqs out of ksoftirqd<--
> >> *** fail ***

Looking at the places where HRTIMER_SOFTIRQ is raised, it did look like
there is at least one case where a hrtimer started from process context
would cause HRTIMER_SOFTIRQ to be set pending, but the associated
ktimersoftirq/N not woken, which seems problematic.

  Josh

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 7abfdab..d91f378 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -749,7 +749,7 @@ void raise_softirq_irqoff(unsigned int nr)
 *
 */
if (!current->softirq_nestcnt)
-   wakeup_softirqd();
+   wakeup_proper_softirq(nr);
 }
 
 static inline int ksoftirqd_softirq_pending(void)

Re: [ANNOUNCE] 3.14.64-rt67

[Josh Cartwright]

On Tue, Mar 15, 2016 at 10:50:31PM -0400, Paul Gortmaker wrote:
> On Tue, Mar 15, 2016 at 7:25 PM, Paul Gortmaker
>  wrote:
> > On Tue, Mar 15, 2016 at 5:45 PM, Paul Gortmaker
> >  wrote:
> >> On Mon, Mar 14, 2016 at 11:49 AM, Steven Rostedt  
> >> wrote:
> >>>
> >>> Dear RT Folks,
> >>>
> >>> 3.14 release on PI(E) Day!
> >>>
> >>> I'm pleased to announce the 3.14.64-rt67 stable release.
> >>
> >> Testing this with what is largely a x86-64 defconfig but with RT_FULL,
> >> I now see:
> >>
> >> root@dell760-paul:~# dmesg|grep NOH
> >> [8.605854] NOHZ: local_softirq_pending 100
> >> [8.732677] NOHZ: local_softirq_pending 100
> >> [8.852729] NOHZ: local_softirq_pending 100
> >> [8.963964] NOHZ: local_softirq_pending 100
> >> [9.061892] NOHZ: local_softirq_pending 100
> >> [9.184921] NOHZ: local_softirq_pending 100
> >> [9.370958] NOHZ: local_softirq_pending 100
> >> [9.657811] NOHZ: local_softirq_pending 100
> >> [9.942631] NOHZ: local_softirq_pending 100
> >> [   10.783710] NOHZ: local_softirq_pending 100
> >> root@dell760-paul:~#
> >>
> >> ...early in boot (we cap them after ~10 msgs).
> >>
> >> I think 100 is RCU if I did my bit counting properly; remind
> >> me to submit a patch that uses the human readable names.

As mentioned on IRC, 100 is HRTIMER_SOFTIRQ, which I think makes more
sense...at least it meshes better with the commit you identified as
being problematic.

> >>
> >> I had a good hunch which commit was responsible but I did
> >> a check of it and the one directly underneath it to be sure,
> >> and the latter boots w/o any pending messages.
> >>
> >> git log --oneline  v3.14-rt ^v3.14.64
> >> [...]
> >> 0a80a6849f19 latencyhist: disable jump-labels
> >> a884ef48e1ca net: provide a way to delegate processing a softirq to 
> >> ksoftirqd
> >> 780d7ca2fdb0 softirq: split timer softirqs out of ksoftirqd<--
> >> *** fail ***

Looking at the places where HRTIMER_SOFTIRQ is raised, it did look like
there is at least one case where a hrtimer started from process context
would cause HRTIMER_SOFTIRQ to be set pending, but the associated
ktimersoftirq/N not woken, which seems problematic.

  Josh

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 7abfdab..d91f378 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -749,7 +749,7 @@ void raise_softirq_irqoff(unsigned int nr)
 *
 */
if (!current->softirq_nestcnt)
-   wakeup_softirqd();
+   wakeup_proper_softirq(nr);
 }
 
 static inline int ksoftirqd_softirq_pending(void)

Re: [PATCH 2/2] misc: nirtfeatures: physical interface elements

[Josh Cartwright]

Hey Kyle-

On Mon, Mar 14, 2016 at 04:55:08PM -0500, Kyle Roeschley wrote:
> From: Gratian Crisan 

From what I understand, this was mostly Aaron's work, so he should get
authorship.  I could be wrong, though, but you'll want to check.

> These changes add support for PIEs (physical interface elements), which
> are defined as physical elements fixed to a controller/chassis with
> which a user can interact (e.g. LEDs and switches) and whose meaning
> is user-defined and implementation-specific.
[..]
> ---
>  drivers/misc/nirtfeatures.c | 753 
> 
>  1 file changed, 694 insertions(+), 59 deletions(-)

This patchset is awkwardly split up, especially because you are removing
what you are adding in the first patch.  I would suggest coming up with
a better patch breakdown that doesn't do this, to make it easier on
reviewers.

Perhaps, breaking it up into a patchset where each patch implements a
different class of functionality (leds, input).

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 2/2] misc: nirtfeatures: physical interface elements

[Josh Cartwright]

Hey Kyle-

On Mon, Mar 14, 2016 at 04:55:08PM -0500, Kyle Roeschley wrote:
> From: Gratian Crisan 

From what I understand, this was mostly Aaron's work, so he should get
authorship.  I could be wrong, though, but you'll want to check.

> These changes add support for PIEs (physical interface elements), which
> are defined as physical elements fixed to a controller/chassis with
> which a user can interact (e.g. LEDs and switches) and whose meaning
> is user-defined and implementation-specific.
[..]
> ---
>  drivers/misc/nirtfeatures.c | 753 
> 
>  1 file changed, 694 insertions(+), 59 deletions(-)

This patchset is awkwardly split up, especially because you are removing
what you are adding in the first patch.  I would suggest coming up with
a better patch breakdown that doesn't do this, to make it easier on
reviewers.

Perhaps, breaking it up into a patchset where each patch implements a
different class of functionality (leds, input).

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 1/2] misc: add nirtfeatures driver

[Josh Cartwright]

On Mon, Mar 14, 2016 at 03:05:59PM -0700, Greg KH wrote:
> On Mon, Mar 14, 2016 at 04:54:32PM -0500, Kyle Roeschley wrote:
> > From: Jeff Westfahl 
> > 
> > This driver introduces support for hardware features of National
> > Instruments real-time controllers. This is an ACPI device that exposes
> > LEDs, switches, and watchdogs.
> 
> If it's an acpi driver, why not put it in drivers/acpi?

For the same reason we don't move all drivers for devices-on-a-PCI-bus
into drivers/pci?

Drivers typically exist in the sourcetree with other drivers which
implement similar functionality, which works great for devices with
clear functional boundaries (GPIO controller drivers in drivers/gpio,
led drivers in drivers/leds, etc. etc.); but for devices which are a
hodgepodge of functionality, there isn't really a good fit anywhere
except maybe in misc or mfd.

We could move it to mfd, but drivers in drivers/mfd which don't make use
of MFD_CORE seems equally strange (although, I suppose there is
precedent).  Maybe Lee has some thoughts.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 1/2] misc: add nirtfeatures driver

[Josh Cartwright]

On Mon, Mar 14, 2016 at 03:05:59PM -0700, Greg KH wrote:
> On Mon, Mar 14, 2016 at 04:54:32PM -0500, Kyle Roeschley wrote:
> > From: Jeff Westfahl 
> > 
> > This driver introduces support for hardware features of National
> > Instruments real-time controllers. This is an ACPI device that exposes
> > LEDs, switches, and watchdogs.
> 
> If it's an acpi driver, why not put it in drivers/acpi?

For the same reason we don't move all drivers for devices-on-a-PCI-bus
into drivers/pci?

Drivers typically exist in the sourcetree with other drivers which
implement similar functionality, which works great for devices with
clear functional boundaries (GPIO controller drivers in drivers/gpio,
led drivers in drivers/leds, etc. etc.); but for devices which are a
hodgepodge of functionality, there isn't really a good fit anywhere
except maybe in misc or mfd.

We could move it to mfd, but drivers in drivers/mfd which don't make use
of MFD_CORE seems equally strange (although, I suppose there is
precedent).  Maybe Lee has some thoughts.

  Josh


signature.asc
Description: PGP signature

Re: [RFC v0] Use swait in completion

[Josh Cartwright]

On Tue, Mar 08, 2016 at 06:52:06PM +0100, Sebastian Andrzej Siewior wrote:
> * Daniel Wagner | 2016-03-08 16:59:13 [+0100]:
> 
> >Hi,
> Hi,
> 
> >As Peter correctly pointed out in [1] a simple conversion from
> >wait to swait in completion.c wont work. I played a bit around and
> >came up with this rather ugly idea.
> 
> besides all the things I mentioned privatly, here is what I have
> currently in -RT:
> 
> +void swake_up_all_locked(struct swait_queue_head *q)
> +{
> +   struct swait_queue *curr;
> +   int wakes = 0;
> +
> +   while (!list_empty(>task_list)) {
> +
> +   curr = list_first_entry(>task_list, typeof(*curr),
> +   task_list);
> +   wake_up_process(curr->task);
> +   list_del_init(>task_list);
> +   wakes++;
> +   }
> +   WARN_ON(wakes > 2);
> +}
> +EXPORT_SYMBOL(swake_up_all_locked);
> 
> the remaining part is what you have. The only user so far is complete()
> and currently I see ony complete_all() with zero or one waiter.
> If none of my boxes die over the night, I intend to release this
> tomorrow in -RT and see if someone else triggers the limit.
> 
> However I don't think if your DEFER flag solution is all that bad.  I
> have also the block-mq in -RT using swait and they perform wakes with
> irqs-off. Not in -RT but mainline. So me might need something to make
> it work properly. But if we defer the wakeup they might come at us and
> complain about the latency???

Is it really just about latency?  Does this deferral not lead to an
inversion in the case where the single woken task isn't the highest
priority waiter on the completion (and doesn't run due to a
middle-priority thing spinning)?

In order for this to work, it seems like the chosen waiter would need to
inherit the highest priority of all waiters (which AFAICT isn't
happening).

  Josh


signature.asc
Description: PGP signature

Re: [RFC v0] Use swait in completion

[Josh Cartwright]

On Tue, Mar 08, 2016 at 06:52:06PM +0100, Sebastian Andrzej Siewior wrote:
> * Daniel Wagner | 2016-03-08 16:59:13 [+0100]:
> 
> >Hi,
> Hi,
> 
> >As Peter correctly pointed out in [1] a simple conversion from
> >wait to swait in completion.c wont work. I played a bit around and
> >came up with this rather ugly idea.
> 
> besides all the things I mentioned privatly, here is what I have
> currently in -RT:
> 
> +void swake_up_all_locked(struct swait_queue_head *q)
> +{
> +   struct swait_queue *curr;
> +   int wakes = 0;
> +
> +   while (!list_empty(>task_list)) {
> +
> +   curr = list_first_entry(>task_list, typeof(*curr),
> +   task_list);
> +   wake_up_process(curr->task);
> +   list_del_init(>task_list);
> +   wakes++;
> +   }
> +   WARN_ON(wakes > 2);
> +}
> +EXPORT_SYMBOL(swake_up_all_locked);
> 
> the remaining part is what you have. The only user so far is complete()
> and currently I see ony complete_all() with zero or one waiter.
> If none of my boxes die over the night, I intend to release this
> tomorrow in -RT and see if someone else triggers the limit.
> 
> However I don't think if your DEFER flag solution is all that bad.  I
> have also the block-mq in -RT using swait and they perform wakes with
> irqs-off. Not in -RT but mainline. So me might need something to make
> it work properly. But if we defer the wakeup they might come at us and
> complain about the latency???

Is it really just about latency?  Does this deferral not lead to an
inversion in the case where the single woken task isn't the highest
priority waiter on the completion (and doesn't run due to a
middle-priority thing spinning)?

In order for this to work, it seems like the chosen waiter would need to
inherit the highest priority of all waiters (which AFAICT isn't
happening).

  Josh


signature.asc
Description: PGP signature

[PATCH] sc16is7xx: drop bogus use of IRQF_ONESHOT

[Josh Cartwright]

The use of IRQF_ONESHOT when registering an interrupt handler with
request_irq() is non-sensical.

Not only that, it also prevents the handler from being threaded when it
otherwise should be w/ IRQ_FORCED_THREADING is enabled.  This causes the
following deadlock observed by Sean Nyekjaer on -rt:

Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[..]
   rt_spin_lock_slowlock from (queue_kthread_work+0x18/0x74)
   queue_kthread_work) from (sc16is7xx_irq+0x10/0x18 [sc16is7xx])
   sc16is7xx_irq [sc16is7xx]) from (handle_irq_event_percpu+0x70/0x158)
   handle_irq_event_percpu) from (handle_irq_event+0x68/0xa8)
   handle_irq_event) from (handle_level_irq+0x10c/0x184)
   handle_level_irq) from (generic_handle_irq+0x2c/0x3c)
   generic_handle_irq) from (mxc_gpio_irq_handler+0x3c/0x108)
   mxc_gpio_irq_handler) from (mx3_gpio_irq_handler+0x80/0xcc)
   mx3_gpio_irq_handler) from (generic_handle_irq+0x2c/0x3c)
   generic_handle_irq) from (__handle_domain_irq+0x7c/0xe8)
   __handle_domain_irq) from (gic_handle_irq+0x24/0x5c)
   gic_handle_irq) from (__irq_svc+0x40/0x88)
   (__irq_svc) from (rt_spin_unlock+0x1c/0x68)
   (rt_spin_unlock) from (kthread_worker_fn+0x104/0x17c)
   (kthread_worker_fn) from (kthread+0xd0/0xe8)
   (kthread) from (ret_from_fork+0x14/0x2c)

Reported-by: Sean Nyekjaer <sean.nyekj...@prevas.dk>
Fixes: 9e6f4ca3e567 ("sc16is7xx: use kthread_worker for tx_work and irq")
Cc: sta...@vger.kernel.org # v4.1+
Signed-off-by: Josh Cartwright <jo...@ni.com>
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e78fa99..1bb5c0e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1257,7 +1257,7 @@ static int sc16is7xx_probe(struct device *dev,
 
/* Setup interrupt */
ret = devm_request_irq(dev, irq, sc16is7xx_irq,
-  IRQF_ONESHOT | flags, dev_name(dev), s);
+  flags, dev_name(dev), s);
if (!ret)
return 0;
 
-- 
2.7.0

[PATCH] sc16is7xx: drop bogus use of IRQF_ONESHOT

[Josh Cartwright]

The use of IRQF_ONESHOT when registering an interrupt handler with
request_irq() is non-sensical.

Not only that, it also prevents the handler from being threaded when it
otherwise should be w/ IRQ_FORCED_THREADING is enabled.  This causes the
following deadlock observed by Sean Nyekjaer on -rt:

Internal error: Oops - BUG: 0 [#1] PREEMPT SMP ARM
[..]
   rt_spin_lock_slowlock from (queue_kthread_work+0x18/0x74)
   queue_kthread_work) from (sc16is7xx_irq+0x10/0x18 [sc16is7xx])
   sc16is7xx_irq [sc16is7xx]) from (handle_irq_event_percpu+0x70/0x158)
   handle_irq_event_percpu) from (handle_irq_event+0x68/0xa8)
   handle_irq_event) from (handle_level_irq+0x10c/0x184)
   handle_level_irq) from (generic_handle_irq+0x2c/0x3c)
   generic_handle_irq) from (mxc_gpio_irq_handler+0x3c/0x108)
   mxc_gpio_irq_handler) from (mx3_gpio_irq_handler+0x80/0xcc)
   mx3_gpio_irq_handler) from (generic_handle_irq+0x2c/0x3c)
   generic_handle_irq) from (__handle_domain_irq+0x7c/0xe8)
   __handle_domain_irq) from (gic_handle_irq+0x24/0x5c)
   gic_handle_irq) from (__irq_svc+0x40/0x88)
   (__irq_svc) from (rt_spin_unlock+0x1c/0x68)
   (rt_spin_unlock) from (kthread_worker_fn+0x104/0x17c)
   (kthread_worker_fn) from (kthread+0xd0/0xe8)
   (kthread) from (ret_from_fork+0x14/0x2c)

Reported-by: Sean Nyekjaer 
Fixes: 9e6f4ca3e567 ("sc16is7xx: use kthread_worker for tx_work and irq")
Cc: sta...@vger.kernel.org # v4.1+
Signed-off-by: Josh Cartwright 
---
 drivers/tty/serial/sc16is7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e78fa99..1bb5c0e 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1257,7 +1257,7 @@ static int sc16is7xx_probe(struct device *dev,
 
/* Setup interrupt */
ret = devm_request_irq(dev, irq, sc16is7xx_irq,
-  IRQF_ONESHOT | flags, dev_name(dev), s);
+  flags, dev_name(dev), s);
if (!ret)
return 0;
 
-- 
2.7.0

Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

[Josh Cartwright]

Hey Alan-

First off, thanks for all of your (and others') work on this.

On Fri, Feb 05, 2016 at 03:29:58PM -0600, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> New bindings document for FPGA Region to support programming
> FPGA's under Device Tree control
> 
> Signed-off-by: Alan Tull 
> Signed-off-by: Moritz Fischer 
[..]
> ---
>  .../devicetree/bindings/fpga/fpga-region.txt   |  348 
> 
>  1 file changed, 348 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt 
> b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> new file mode 100644
> index 000..ccd7127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
[..]
> +FPGA Manager & FPGA Manager Framework
> + * An FPGA Manager is a hardware block that programs an FPGA under the 
> control
> +   of a host processor.
> + * The FPGA Manager Framework provides drivers and functions to program an
> +   FPGA.
> +
> +FPGA Bridge Framework
> + * Provides drivers and functions to control bridges that enable/disable
> +   data to the FPGA.
> + * FPGA Bridges should be disabled while the FPGA is being programmed to
> +   prevent spurious data on the bus.
> + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> +   handles this.

It still seems strange for me architecturally for the FPGA Bridge to be
a first-class top-level concept in your architecture, as they are a
reflection of the SoC FPGA manager design.  That is, I would expect the
bridges not to be associated with the FPGA Region, but with the FPGA
manager.

Although, I will concede that you you've made it possible to not use
FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
doesn't matter, just smells strangely.

> +Freeze Blocks
> + * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the 
> case
> +   of PR, the buses from the processor are split within the FPGA.  Each PR
> +   region gets its own split of the buses, protected by an independently
> +   controlled Freeze Block.  Several busses may be connected to a single
> +   PR region; a Freeze Block controls the traffic of all these busses
> +   together.
> +
> +
[..]
> +Device Tree Examples
> +
> +
> +The intention of this section is to give some simple examples, focusing on
> +the placement of the elements detailed above, especially:
> + * FPGA Manager
> + * FPGA Bridges
> + * FPGA Region
> + * ranges
> + * target-path or target
> +
> +For the purposes of this section, I'm dividing the Device Tree into two 
> parts,
> +each with its own requirements.  The two parts are:
> + * The live DT prior to the overlay being added
> + * The DT overlay
> +
> +The live Device Tree must contain an FPGA Region, an FPGA Manager, and any 
> FPGA
> +Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by 
> phandle
> +to handle programming the FPGA.  If the FPGA Region is the child of another 
> FPGA
> +Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be 
> involved,
> +they are specified in the FPGA Region by the "fpga-bridges" property.  During
> +FPGA programming, the FPGA Region will disable the bridges that are in its
> +"fpga-bridges" list and will re-enable them after FPGA programming has
> +succeeded.
> +
> +The Device Tree Overlay will contain:
> + * "target-path" or "target"
> +   The insertion point where the the contents of the overlay will go into the
> +   live tree.  target-path is a full path, while target is a phandle.
> + * "ranges"
> + * "firmware-name"
> +   Specifies the name of the FPGA image file on the firmware search
> +   path.  The search path is described in the firmware class documentation.
> + * "partial-reconfig"
> +   This binding is a boolean and should be present if partial reconfiguration
> +   is to be done.

Another architectural smell: there are categorically two different types
of properties here.  The first kind is those properties which describe
_what_ IP exists within the FPGA image (all of the nodes under the regions, 
etc).
The second kind of properties are those which describe _how_ the image
should be written (partial-reconfig, firmware-name).

It seems weird, but maybe it doesn't matter much.

Thanks,
  Josh


signature.asc
Description: PGP signature

Re: [PATCH v16 1/6] fpga: add bindings document for fpga region

[Josh Cartwright]

Hey Alan-

First off, thanks for all of your (and others') work on this.

On Fri, Feb 05, 2016 at 03:29:58PM -0600, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> New bindings document for FPGA Region to support programming
> FPGA's under Device Tree control
> 
> Signed-off-by: Alan Tull 
> Signed-off-by: Moritz Fischer 
[..]
> ---
>  .../devicetree/bindings/fpga/fpga-region.txt   |  348 
> 
>  1 file changed, 348 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/fpga-region.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/fpga-region.txt 
> b/Documentation/devicetree/bindings/fpga/fpga-region.txt
> new file mode 100644
> index 000..ccd7127
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/fpga-region.txt
[..]
> +FPGA Manager & FPGA Manager Framework
> + * An FPGA Manager is a hardware block that programs an FPGA under the 
> control
> +   of a host processor.
> + * The FPGA Manager Framework provides drivers and functions to program an
> +   FPGA.
> +
> +FPGA Bridge Framework
> + * Provides drivers and functions to control bridges that enable/disable
> +   data to the FPGA.
> + * FPGA Bridges should be disabled while the FPGA is being programmed to
> +   prevent spurious data on the bus.
> + * FPGA Bridges may not be needed in implementations where the FPGA Manager
> +   handles this.

It still seems strange for me architecturally for the FPGA Bridge to be
a first-class top-level concept in your architecture, as they are a
reflection of the SoC FPGA manager design.  That is, I would expect the
bridges not to be associated with the FPGA Region, but with the FPGA
manager.

Although, I will concede that you you've made it possible to not use
FPGA Bridges (like on Zynq where they aren't necessary), so maybe it
doesn't matter, just smells strangely.

> +Freeze Blocks
> + * Freeze Blocks function as FPGA Bridges within the FPGA fabric.  In the 
> case
> +   of PR, the buses from the processor are split within the FPGA.  Each PR
> +   region gets its own split of the buses, protected by an independently
> +   controlled Freeze Block.  Several busses may be connected to a single
> +   PR region; a Freeze Block controls the traffic of all these busses
> +   together.
> +
> +
[..]
> +Device Tree Examples
> +
> +
> +The intention of this section is to give some simple examples, focusing on
> +the placement of the elements detailed above, especially:
> + * FPGA Manager
> + * FPGA Bridges
> + * FPGA Region
> + * ranges
> + * target-path or target
> +
> +For the purposes of this section, I'm dividing the Device Tree into two 
> parts,
> +each with its own requirements.  The two parts are:
> + * The live DT prior to the overlay being added
> + * The DT overlay
> +
> +The live Device Tree must contain an FPGA Region, an FPGA Manager, and any 
> FPGA
> +Bridges.  The FPGA Region's "fpga-mgr" property specifies the manager by 
> phandle
> +to handle programming the FPGA.  If the FPGA Region is the child of another 
> FPGA
> +Region, the parent's FPGA Manager is used.  If FPGA Bridges need to be 
> involved,
> +they are specified in the FPGA Region by the "fpga-bridges" property.  During
> +FPGA programming, the FPGA Region will disable the bridges that are in its
> +"fpga-bridges" list and will re-enable them after FPGA programming has
> +succeeded.
> +
> +The Device Tree Overlay will contain:
> + * "target-path" or "target"
> +   The insertion point where the the contents of the overlay will go into the
> +   live tree.  target-path is a full path, while target is a phandle.
> + * "ranges"
> + * "firmware-name"
> +   Specifies the name of the FPGA image file on the firmware search
> +   path.  The search path is described in the firmware class documentation.
> + * "partial-reconfig"
> +   This binding is a boolean and should be present if partial reconfiguration
> +   is to be done.

Another architectural smell: there are categorically two different types
of properties here.  The first kind is those properties which describe
_what_ IP exists within the FPGA image (all of the nodes under the regions, 
etc).
The second kind of properties are those which describe _how_ the image
should be written (partial-reconfig, firmware-name).

It seems weird, but maybe it doesn't matter much.

Thanks,
  Josh


signature.asc
Description: PGP signature

[PATCH 2/2] ARM: zynq: address L2 cache data corruption

[Josh Cartwright]

The Zynq has a bug where the L2 cache will return invalid data in some
circumstances unless the L2C_RAM register is set to 0x00020202 before the first
enabling of the L2 cache.

The Xilinx-recommended solution to this problem is to ensure that early one of
the earlier bootstages correctly initialize L2C_RAM, however, this issue wasn't
discovered and fixed until after their EDK/SDK 14.4 release.  For systems built
prior to that, and which lack field-upgradable bootloaders, this issue still
exists and silent data corruption can be seen in the wild.

Fix these systems by ensuring L2C_RAM is properly initialized at the
earliest convenient moment prior to the L2 being brought up, which is
when the SLCR is first mapped.

The Zynq bug is described in more detail by Xilinx AR# 54190 as quoted
below.

Xilinx AR# 54190
http://www.xilinx.com/support/answers/54190.htm
Captured on 2014-09-24 14:43 -0500

  = Description =
  For proper L2 cache operation, the user code must program the
  slcr.L2C_RAM register (address 0xF800_0A1C) to the value of
  0x0002_0202 before enabling the L2 cache. The reset value
  (0x0001_0101) might cause, very infrequently, the L2 cache to return
  invalid data.

  = Solution =
  It is up to the user code (FSBL or other user code) to set the
  slcr.L2C_RAM register to the value 0x0002_0202 before enabling the L2
  cache.

  Note: The L2 cache is disabled after reset and is not enabled by the
  BootROM.

  Note: The slcr.l2C_RAM register was previously reserved. It is added
  in the Zynq-7000 AP SoC Technical Reference Manual (TRM) v1.5 as
  "Reserved".

Thanks to Jaeden Amero for initial debugging and triage efforts.

Signed-off-by: Josh Cartwright 
---
 arch/arm/mach-zynq/slcr.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index 26320eb..f0292a3 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -28,6 +28,7 @@
 #define SLCR_A9_CPU_RST_CTRL_OFFSET0x244 /* CPU Software Reset Control */
 #define SLCR_REBOOT_STATUS_OFFSET  0x258 /* PS Reboot Status */
 #define SLCR_PSS_IDCODE0x530 /* PS IDCODE */
+#define SLCR_L2C_RAM   0xA1C /* L2C_RAM in AR#54190 */
 
 #define SLCR_UNLOCK_MAGIC  0xDF0D
 #define SLCR_A9_CPU_CLKSTOP0x10
@@ -227,6 +228,9 @@ int __init zynq_early_slcr_init(void)
/* unlock the SLCR so that registers can be changed */
zynq_slcr_unlock();
 
+   /* See AR#54190 design advisory */
+   regmap_update_bits(zynq_slcr_regmap, SLCR_L2C_RAM, 0x70707, 0x20202);
+
register_restart_handler(_slcr_restart_nb);
 
pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
-- 
2.7.0

[PATCH 0/2] ARM: zynq: address silent L2 cache corruption

[Josh Cartwright]

The Zynq has a bug where the L2 cache will return invalid data in some
circumstances unless the L2C_RAM register is set to 0x20202 before the first
enabling of the L2 cache.

The Xilinx-recommended solution to this problem is to ensure that early one of
the earlier bootstages correctly initialize L2C_RAM, however, this issue wasn't
discovered and fixed until after their EDK/SDK 14.4 release.  For systems built
prior to that, and which lack field-upgradable bootloaders, this issue still
exists and silent data corruption can be seen in the wild.

Fix these systems by ensuring L2C_RAM is properly initialized at the
earliest convenient moment prior to the L2 being brought up, which is
when the SLCR is first mapped.

Unfortunately, there isn't much public documentation on exactly what the
L2C_RAM register is for, or how it is used, only that software is responsible
for initializing it to the proper value prior to bringing up L2.

You can find more information about this bug in AR#54190[1].

1: http://www.xilinx.com/support/answers/54190.html

Josh Cartwright (2):
  ARM: zynq: initialize slcr mapping earlier
  ARM: zynq: address L2 cache data corruption

 arch/arm/mach-zynq/common.c | 3 +--
 arch/arm/mach-zynq/slcr.c   | 4 
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.7.0

[PATCH 1/2] ARM: zynq: initialize slcr mapping earlier

[Josh Cartwright]

In preparation for performing additional configuration prior to bringing
up L2, move the slcr initialization earlier in the boot process.

Signed-off-by: Josh Cartwright 
---
 arch/arm/mach-zynq/common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 6f39d03..860ffb6 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -150,8 +150,6 @@ out:
 
 static void __init zynq_timer_init(void)
 {
-   zynq_early_slcr_init();
-
zynq_clock_init();
of_clk_init(NULL);
clocksource_probe();
@@ -186,6 +184,7 @@ static void __init zynq_map_io(void)
 
 static void __init zynq_irq_init(void)
 {
+   zynq_early_slcr_init();
irqchip_init();
 }
 
-- 
2.7.0

[PATCH 2/2] ARM: zynq: address L2 cache data corruption

[Josh Cartwright]

The Zynq has a bug where the L2 cache will return invalid data in some
circumstances unless the L2C_RAM register is set to 0x00020202 before the first
enabling of the L2 cache.

The Xilinx-recommended solution to this problem is to ensure that early one of
the earlier bootstages correctly initialize L2C_RAM, however, this issue wasn't
discovered and fixed until after their EDK/SDK 14.4 release.  For systems built
prior to that, and which lack field-upgradable bootloaders, this issue still
exists and silent data corruption can be seen in the wild.

Fix these systems by ensuring L2C_RAM is properly initialized at the
earliest convenient moment prior to the L2 being brought up, which is
when the SLCR is first mapped.

The Zynq bug is described in more detail by Xilinx AR# 54190 as quoted
below.

Xilinx AR# 54190
http://www.xilinx.com/support/answers/54190.htm
Captured on 2014-09-24 14:43 -0500

  = Description =
  For proper L2 cache operation, the user code must program the
  slcr.L2C_RAM register (address 0xF800_0A1C) to the value of
  0x0002_0202 before enabling the L2 cache. The reset value
  (0x0001_0101) might cause, very infrequently, the L2 cache to return
  invalid data.

  = Solution =
  It is up to the user code (FSBL or other user code) to set the
  slcr.L2C_RAM register to the value 0x0002_0202 before enabling the L2
  cache.

  Note: The L2 cache is disabled after reset and is not enabled by the
  BootROM.

  Note: The slcr.l2C_RAM register was previously reserved. It is added
  in the Zynq-7000 AP SoC Technical Reference Manual (TRM) v1.5 as
  "Reserved".

Thanks to Jaeden Amero for initial debugging and triage efforts.

Signed-off-by: Josh Cartwright <jo...@ni.com>
---
 arch/arm/mach-zynq/slcr.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-zynq/slcr.c b/arch/arm/mach-zynq/slcr.c
index 26320eb..f0292a3 100644
--- a/arch/arm/mach-zynq/slcr.c
+++ b/arch/arm/mach-zynq/slcr.c
@@ -28,6 +28,7 @@
 #define SLCR_A9_CPU_RST_CTRL_OFFSET0x244 /* CPU Software Reset Control */
 #define SLCR_REBOOT_STATUS_OFFSET  0x258 /* PS Reboot Status */
 #define SLCR_PSS_IDCODE0x530 /* PS IDCODE */
+#define SLCR_L2C_RAM   0xA1C /* L2C_RAM in AR#54190 */
 
 #define SLCR_UNLOCK_MAGIC  0xDF0D
 #define SLCR_A9_CPU_CLKSTOP0x10
@@ -227,6 +228,9 @@ int __init zynq_early_slcr_init(void)
/* unlock the SLCR so that registers can be changed */
zynq_slcr_unlock();
 
+   /* See AR#54190 design advisory */
+   regmap_update_bits(zynq_slcr_regmap, SLCR_L2C_RAM, 0x70707, 0x20202);
+
register_restart_handler(_slcr_restart_nb);
 
pr_info("%s mapped to %p\n", np->name, zynq_slcr_base);
-- 
2.7.0

[PATCH 1/2] ARM: zynq: initialize slcr mapping earlier

[Josh Cartwright]

In preparation for performing additional configuration prior to bringing
up L2, move the slcr initialization earlier in the boot process.

Signed-off-by: Josh Cartwright <jo...@ni.com>
---
 arch/arm/mach-zynq/common.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/mach-zynq/common.c b/arch/arm/mach-zynq/common.c
index 6f39d03..860ffb6 100644
--- a/arch/arm/mach-zynq/common.c
+++ b/arch/arm/mach-zynq/common.c
@@ -150,8 +150,6 @@ out:
 
 static void __init zynq_timer_init(void)
 {
-   zynq_early_slcr_init();
-
zynq_clock_init();
of_clk_init(NULL);
clocksource_probe();
@@ -186,6 +184,7 @@ static void __init zynq_map_io(void)
 
 static void __init zynq_irq_init(void)
 {
+   zynq_early_slcr_init();
irqchip_init();
 }
 
-- 
2.7.0

[PATCH 0/2] ARM: zynq: address silent L2 cache corruption

[Josh Cartwright]

The Zynq has a bug where the L2 cache will return invalid data in some
circumstances unless the L2C_RAM register is set to 0x20202 before the first
enabling of the L2 cache.

The Xilinx-recommended solution to this problem is to ensure that early one of
the earlier bootstages correctly initialize L2C_RAM, however, this issue wasn't
discovered and fixed until after their EDK/SDK 14.4 release.  For systems built
prior to that, and which lack field-upgradable bootloaders, this issue still
exists and silent data corruption can be seen in the wild.

Fix these systems by ensuring L2C_RAM is properly initialized at the
earliest convenient moment prior to the L2 being brought up, which is
when the SLCR is first mapped.

Unfortunately, there isn't much public documentation on exactly what the
L2C_RAM register is for, or how it is used, only that software is responsible
for initializing it to the proper value prior to bringing up L2.

You can find more information about this bug in AR#54190[1].

1: http://www.xilinx.com/support/answers/54190.html

Josh Cartwright (2):
  ARM: zynq: initialize slcr mapping earlier
  ARM: zynq: address L2 cache data corruption

 arch/arm/mach-zynq/common.c | 3 +--
 arch/arm/mach-zynq/slcr.c   | 4 
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.7.0

Re: [PATCH net 1/2] net: cadence: macb: Disable USRIO register on some platforms

[Josh Cartwright]

On Mon, Dec 07, 2015 at 11:58:33AM +0100, Neil Armstrong wrote:
> On some platforms, the macb integration does not use the USRIO
> register to configure the (R)MII port and clocks.
> When the register is not implemented and the MACB error signal
> is connected to the bus error, reading or writing to the USRIO
> register can trigger some Imprecise External Aborts on ARM platforms.
> ---

Does this make sense to even be a separate bool device tree property?

This sort of configuration is typically done by:
   1. Creating a new 'caps' bit; relevant codepaths check that bit
   2. Creating a new "compatible" string for your platform's macb
  instance
   3. Creating a new 'struct macb_config' instance for your platform,
  setting any relevant caps bits when it is selected.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH net 1/2] net: cadence: macb: Disable USRIO register on some platforms

[Josh Cartwright]

On Mon, Dec 07, 2015 at 11:58:33AM +0100, Neil Armstrong wrote:
> On some platforms, the macb integration does not use the USRIO
> register to configure the (R)MII port and clocks.
> When the register is not implemented and the MACB error signal
> is connected to the bus error, reading or writing to the USRIO
> register can trigger some Imprecise External Aborts on ARM platforms.
> ---

Does this make sense to even be a separate bool device tree property?

This sort of configuration is typically done by:
   1. Creating a new 'caps' bit; relevant codepaths check that bit
   2. Creating a new "compatible" string for your platform's macb
  instance
   3. Creating a new 'struct macb_config' instance for your platform,
  setting any relevant caps bits when it is selected.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Josh Cartwright]

On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote:
> On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
> Jiri Kosina  wrote:
> 
> > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> > 
> > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > big and in rare cases causes a recursive deadlock because of its call
> > > to hid_input_report().
> > > 
> > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > is that it submits a report to reschedule a proximity read through a
> > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > before calling hid_input_report(). When the irq kicks in on the same
> > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > 
> > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > protect only the instructions which modify usbhid, thus move the lock
> > > after the hid_input_report() call and the deadlock dissapears.  
> > 
> > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > isn't it?
> > 
> 
> That was my first attempt, yes, but the deadlock still happens with interrupts
> disabled. It is very weird, I know.

I think your best course of action is to figure out why this is the
case, instead of continuing with trying to solve the symptoms.  Do you
have actual callstacks showing the cases where you hit?  That might be
useful to share (your lockdep picture cuts out the callstacks).

Also, have you tried without the PREEMPT_RT patch in the picture at all?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock

[Josh Cartwright]

On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote:
> On Wed, 18 Nov 2015 21:37:42 +0100 (CET)
> Jiri Kosina  wrote:
> 
> > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote:
> > 
> > > The critical section protected by usbhid->lock in hid_ctrl() is too
> > > big and in rare cases causes a recursive deadlock because of its call
> > > to hid_input_report().
> > > 
> > > This deadlock reproduces on newer wacom tablets like 056a:033c because
> > > the wacom driver in its irq handler ends up calling hid_hw_request()
> > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
> > > is that it submits a report to reschedule a proximity read through a
> > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
> > > before calling hid_input_report(). When the irq kicks in on the same
> > > cpu, it also tries to grab the lock resulting in a recursive deadlock.
> > > 
> > > The proper fix is to shrink the critical section in hid_ctrl() to
> > > protect only the instructions which modify usbhid, thus move the lock
> > > after the hid_input_report() call and the deadlock dissapears.  
> > 
> > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), 
> > isn't it?
> > 
> 
> That was my first attempt, yes, but the deadlock still happens with interrupts
> disabled. It is very weird, I know.

I think your best course of action is to figure out why this is the
case, instead of continuing with trying to solve the symptoms.  Do you
have actual callstacks showing the cases where you hit?  That might be
useful to share (your lockdep picture cuts out the callstacks).

Also, have you tried without the PREEMPT_RT patch in the picture at all?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:59:16PM -0500, Josh Cartwright wrote:
> On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> > On Wed, 28 Oct 2015, Moritz Fischer wrote:
> > 
> > > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright  wrote:
> > > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright  wrote:
> > > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, 
> > > >> > at...@opensource.altera.com wrote:
> > > >> >> From: Alan Tull 
> > > >> >>
> > > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > > >> >> interface for reprogramming FPGAs that is Device Tree
> > > >> >> Overlays-based.
> > > >> >
> > > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq 
> > > >> > at
> > > >> > all, from what I can tell.
> > > >>
> > > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > > >> reconfiguration,
> > > >> and doesn't for partial reconfiguration. Now depending on which parts
> > > >> of the fabric
> > > >> are partial reconfigured (say AXI masters), one might run into issues
> > > >> with a setup like that.
> > > >>
> > > >> My first plan was to counter that by using zynq-reset to hold the
> > > >> reset high during
> > > >> reconfiguration of that part of the FPGA.
> > > >>
> > > >> I'm happy to rethink that part and maybe redo the level shifters and
> > > >> resets together in a bridge
> > > >> driver under devicetree control gives finer grained control.
> > > >
> > > > There is already a framework which is used to describe and manipulate
> > > > level shifting/other IO properties, and that is pinctrl, and if we
> > > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > > best bet.
> > > 
> > > Alright, I'll investigate that. Again, for the non-partial reconfig
> > > case I'm happy
> > > with the behavior as implemented, for the partial reconfig I just
> > > haven't run into
> > > issues with not dealing with the level shifters.
> > 
> > Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
> 
> I'm suggesting that for the set of operations/configuration states that
> need to be managed _for the Zynq[1]_ during reprogramming, I think
> pinctrl might be a good fit.
> 
> But the pinctrl state activation would happen in the context of the zynq
> fpga_mgr_ops write

*grr*... in the context of the zynq's write_init(), and write_complete()
callbacks.

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> On Wed, 28 Oct 2015, Moritz Fischer wrote:
> 
> > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright  wrote:
> > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright  wrote:
> > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com 
> > >> > wrote:
> > >> >> From: Alan Tull 
> > >> >>
> > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > >> >> interface for reprogramming FPGAs that is Device Tree
> > >> >> Overlays-based.
> > >> >
> > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > >> > all, from what I can tell.
> > >>
> > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > >> reconfiguration,
> > >> and doesn't for partial reconfiguration. Now depending on which parts
> > >> of the fabric
> > >> are partial reconfigured (say AXI masters), one might run into issues
> > >> with a setup like that.
> > >>
> > >> My first plan was to counter that by using zynq-reset to hold the
> > >> reset high during
> > >> reconfiguration of that part of the FPGA.
> > >>
> > >> I'm happy to rethink that part and maybe redo the level shifters and
> > >> resets together in a bridge
> > >> driver under devicetree control gives finer grained control.
> > >
> > > There is already a framework which is used to describe and manipulate
> > > level shifting/other IO properties, and that is pinctrl, and if we
> > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > best bet.
> > 
> > Alright, I'll investigate that. Again, for the non-partial reconfig
> > case I'm happy
> > with the behavior as implemented, for the partial reconfig I just
> > haven't run into
> > issues with not dealing with the level shifters.
> 
> Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?

I'm suggesting that for the set of operations/configuration states that
need to be managed _for the Zynq[1]_ during reprogramming, I think
pinctrl might be a good fit.

But the pinctrl state activation would happen in the context of the zynq
fpga_mgr_ops write

I do not think it's a good fit for the socfpga, or for the lower level
fpga drivers _in general_.  Nor do I think that the FPGA Bridge
framework, as written, is a good fit for fpgas in general.

  Josh

[1]: Speaking only of the Zynq 7000-series, I don't know anything about
 the fancy new Zynq MPSoC :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright  wrote:
> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com wrote:
> >> From: Alan Tull 
> >>
> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> >> interface for reprogramming FPGAs that is Device Tree
> >> Overlays-based.
> >
> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > all, from what I can tell.
> 
> For Zynq the zynq-fpga driver takes care of the level shifters on full
> reconfiguration,
> and doesn't for partial reconfiguration. Now depending on which parts
> of the fabric
> are partial reconfigured (say AXI masters), one might run into issues
> with a setup like that.
> 
> My first plan was to counter that by using zynq-reset to hold the
> reset high during
> reconfiguration of that part of the FPGA.
> 
> I'm happy to rethink that part and maybe redo the level shifters and
> resets together in a bridge
> driver under devicetree control gives finer grained control.

There is already a framework which is used to describe and manipulate
level shifting/other IO properties, and that is pinctrl, and if we
wanted to use an appropriate abstraction, I think pinctrl would be the
best bet.

Implementing the FPGA Bridge interface in the Zynq driver because it's
what the core expects is just backwards.  It's an abstraction inversion.

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> The Simple FPGA bus uses the FPGA Manager Framework and the
> FPGA Bridge Framework to provide a manufactorer-agnostic
> interface for reprogramming FPGAs that is Device Tree
> Overlays-based.

Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
all, from what I can tell.

Therefore, I would have expected the FPGA Bridge drivers to sit under
the fpga-socfpga driver, and not be a first class feature of the
kernels' FPGA manager subsystem.

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

Re: [PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

On Tue, Oct 27, 2015 at 04:15:59PM -0700, Paul E. McKenney wrote:
> On Tue, Oct 27, 2015 at 08:27:53AM -0700, Eric Dumazet wrote:
> > On Tue, 2015-10-27 at 12:02 -0300, Arnaldo Carvalho de Melo wrote:
[..]
> > > The first suggestion, with it disabled by default seems to be the most
> > > flexible tho, i.e, Paul's original message plus the boot parameter line:
> > > 
> > > Alternatively, a boot-time option could be used:
> > > 
> > > int some_rt_boot_parameter = CONFIG_SYNC_NET_DEFAULT;
> > > 
> > > if (rtnl_is_locked() && !some_rt_boot_parameter)
> > > synchronize_rcu_expedited();
> > > else
> > > synchronize_rcu();
> 
> This could be OK, but why not start with something very simple and automatic?
> We can always add more knobs when and if they actually prove necessary.

I suppose the question is if, for acme's usecases the answer to "when
it's proven necessary" is "now".

> In contrast, unnecessary knobs can cause confusion and might at the same time
> get locked into some misbegotten userspace application, which would make the
> unnecessary knob really hard to get rid of.

I think I would make a stronger statement; the CONFIG_SYNC_NET_DEFAULT
proposed option would be a boot/compile time parameter which says "I
require networking (and network configuration) in my critical path", why
don't we have these flags for other I/O subsystems?  What's special
about networking?

We don't because applications can make use of thread priorities to
express exactly which tasks should be more important than others.  So
perhaps the failure here is that RCU (and networking, by implication)
doesn't (can't?) take into consideration the calling thread's priority?
(And, there may be a cascade of other problems as well, like deferred
work pushed to a waitqueue, and thus losing the callers priority, etc)

(I will admit that RCU is a black box to me, so it is entirely possible
it's already capable of this, or it's fundamentally impossible, or
somewhere in between :)

> > > Then RT oriented kernel .config files would have CONFIG_SYNC_NET_DEFAULT
> > > set to 1, while upstream would have this default to 0.
> > > 
> > > RT oriented kernel users could try using this in some scenarios where
> > > networking is not the critical path.
> > 
> > Well, if synchronize_rcu_expedited() is such a problem on RT, then maybe
> > a generic solution would make synchronize_rcu_expedited() to fallback
> > synchronize_rcu() after boot time on RT.
> > 
> > Not sure why networking use of synchronize_rcu_expedited() would be
> > problematic, and not the others.
> 
> From what I can see, their testing just happened to run into this one.
> Perhaps further testing will run into others, or perhaps the others are
> off in code paths that should not be exercised while running RT apps.

I accidentally ran into this issue when I was doing testing with an
ethernet cable w/ a broken RJ-45 connector (without the tab, that I was
just too lazy to replace), and I kept accidentally knocking it out.  :)

Regardless, industrial automation environments aren't known for having
the most stable network environments; there may be deployed systems
doing high priority motion control tasks, we'd want to ensure that the
poor network technician sent in to repair a defective network switch
wouldn't end up being mangled.

> > scripts/checkpatch.pl has this comment about this :

Also, Documentation/RCU/checklist.txt mentions:

Use of the expedited primitives should be restricted to rare
configuration-change operations that would not normally be
undertaken while a real-time..

I think it could have been argued at the time, that operations under
rtnl_lock() were "configuration-change" operations.  However, for our
use cases, it's not, as link changes are external events beyond control.

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

Re: [PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

On Tue, Oct 27, 2015 at 04:15:59PM -0700, Paul E. McKenney wrote:
> On Tue, Oct 27, 2015 at 08:27:53AM -0700, Eric Dumazet wrote:
> > On Tue, 2015-10-27 at 12:02 -0300, Arnaldo Carvalho de Melo wrote:
[..]
> > > The first suggestion, with it disabled by default seems to be the most
> > > flexible tho, i.e, Paul's original message plus the boot parameter line:
> > > 
> > > Alternatively, a boot-time option could be used:
> > > 
> > > int some_rt_boot_parameter = CONFIG_SYNC_NET_DEFAULT;
> > > 
> > > if (rtnl_is_locked() && !some_rt_boot_parameter)
> > > synchronize_rcu_expedited();
> > > else
> > > synchronize_rcu();
> 
> This could be OK, but why not start with something very simple and automatic?
> We can always add more knobs when and if they actually prove necessary.

I suppose the question is if, for acme's usecases the answer to "when
it's proven necessary" is "now".

> In contrast, unnecessary knobs can cause confusion and might at the same time
> get locked into some misbegotten userspace application, which would make the
> unnecessary knob really hard to get rid of.

I think I would make a stronger statement; the CONFIG_SYNC_NET_DEFAULT
proposed option would be a boot/compile time parameter which says "I
require networking (and network configuration) in my critical path", why
don't we have these flags for other I/O subsystems?  What's special
about networking?

We don't because applications can make use of thread priorities to
express exactly which tasks should be more important than others.  So
perhaps the failure here is that RCU (and networking, by implication)
doesn't (can't?) take into consideration the calling thread's priority?
(And, there may be a cascade of other problems as well, like deferred
work pushed to a waitqueue, and thus losing the callers priority, etc)

(I will admit that RCU is a black box to me, so it is entirely possible
it's already capable of this, or it's fundamentally impossible, or
somewhere in between :)

> > > Then RT oriented kernel .config files would have CONFIG_SYNC_NET_DEFAULT
> > > set to 1, while upstream would have this default to 0.
> > > 
> > > RT oriented kernel users could try using this in some scenarios where
> > > networking is not the critical path.
> > 
> > Well, if synchronize_rcu_expedited() is such a problem on RT, then maybe
> > a generic solution would make synchronize_rcu_expedited() to fallback
> > synchronize_rcu() after boot time on RT.
> > 
> > Not sure why networking use of synchronize_rcu_expedited() would be
> > problematic, and not the others.
> 
> From what I can see, their testing just happened to run into this one.
> Perhaps further testing will run into others, or perhaps the others are
> off in code paths that should not be exercised while running RT apps.

I accidentally ran into this issue when I was doing testing with an
ethernet cable w/ a broken RJ-45 connector (without the tab, that I was
just too lazy to replace), and I kept accidentally knocking it out.  :)

Regardless, industrial automation environments aren't known for having
the most stable network environments; there may be deployed systems
doing high priority motion control tasks, we'd want to ensure that the
poor network technician sent in to repair a defective network switch
wouldn't end up being mangled.

> > scripts/checkpatch.pl has this comment about this :

Also, Documentation/RCU/checklist.txt mentions:

Use of the expedited primitives should be restricted to rare
configuration-change operations that would not normally be
undertaken while a real-time..

I think it could have been argued at the time, that operations under
rtnl_lock() were "configuration-change" operations.  However, for our
use cases, it's not, as link changes are external events beyond control.

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> The Simple FPGA bus uses the FPGA Manager Framework and the
> FPGA Bridge Framework to provide a manufactorer-agnostic
> interface for reprogramming FPGAs that is Device Tree
> Overlays-based.

Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
all, from what I can tell.

Therefore, I would have expected the FPGA Bridge drivers to sit under
the fpga-socfpga driver, and not be a first class feature of the
kernels' FPGA manager subsystem.

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <jo...@ni.com> wrote:
> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com wrote:
> >> From: Alan Tull <at...@opensource.altera.com>
> >>
> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> >> interface for reprogramming FPGAs that is Device Tree
> >> Overlays-based.
> >
> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > all, from what I can tell.
> 
> For Zynq the zynq-fpga driver takes care of the level shifters on full
> reconfiguration,
> and doesn't for partial reconfiguration. Now depending on which parts
> of the fabric
> are partial reconfigured (say AXI masters), one might run into issues
> with a setup like that.
> 
> My first plan was to counter that by using zynq-reset to hold the
> reset high during
> reconfiguration of that part of the FPGA.
> 
> I'm happy to rethink that part and maybe redo the level shifters and
> resets together in a bridge
> driver under devicetree control gives finer grained control.

There is already a framework which is used to describe and manipulate
level shifting/other IO properties, and that is pinctrl, and if we
wanted to use an appropriate abstraction, I think pinctrl would be the
best bet.

Implementing the FPGA Bridge interface in the Zynq driver because it's
what the core expects is just backwards.  It's an abstraction inversion.

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

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> On Wed, 28 Oct 2015, Moritz Fischer wrote:
> 
> > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <jo...@ni.com> wrote:
> > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <jo...@ni.com> wrote:
> > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, at...@opensource.altera.com 
> > >> > wrote:
> > >> >> From: Alan Tull <at...@opensource.altera.com>
> > >> >>
> > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > >> >> interface for reprogramming FPGAs that is Device Tree
> > >> >> Overlays-based.
> > >> >
> > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq at
> > >> > all, from what I can tell.
> > >>
> > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > >> reconfiguration,
> > >> and doesn't for partial reconfiguration. Now depending on which parts
> > >> of the fabric
> > >> are partial reconfigured (say AXI masters), one might run into issues
> > >> with a setup like that.
> > >>
> > >> My first plan was to counter that by using zynq-reset to hold the
> > >> reset high during
> > >> reconfiguration of that part of the FPGA.
> > >>
> > >> I'm happy to rethink that part and maybe redo the level shifters and
> > >> resets together in a bridge
> > >> driver under devicetree control gives finer grained control.
> > >
> > > There is already a framework which is used to describe and manipulate
> > > level shifting/other IO properties, and that is pinctrl, and if we
> > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > best bet.
> > 
> > Alright, I'll investigate that. Again, for the non-partial reconfig
> > case I'm happy
> > with the behavior as implemented, for the partial reconfig I just
> > haven't run into
> > issues with not dealing with the level shifters.
> 
> Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?

I'm suggesting that for the set of operations/configuration states that
need to be managed _for the Zynq[1]_ during reprogramming, I think
pinctrl might be a good fit.

But the pinctrl state activation would happen in the context of the zynq
fpga_mgr_ops write

I do not think it's a good fit for the socfpga, or for the lower level
fpga drivers _in general_.  Nor do I think that the FPGA Bridge
framework, as written, is a good fit for fpgas in general.

  Josh

[1]: Speaking only of the Zynq 7000-series, I don't know anything about
 the fancy new Zynq MPSoC :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v12 3/6] fpga: add simple-fpga-bus

[Josh Cartwright]

On Wed, Oct 28, 2015 at 12:59:16PM -0500, Josh Cartwright wrote:
> On Wed, Oct 28, 2015 at 12:03:41PM -0500, atull wrote:
> > On Wed, 28 Oct 2015, Moritz Fischer wrote:
> > 
> > > On Wed, Oct 28, 2015 at 9:18 AM, Josh Cartwright <jo...@ni.com> wrote:
> > > > On Wed, Oct 28, 2015 at 08:37:51AM -0700, Moritz Fischer wrote:
> > > >> On Wed, Oct 28, 2015 at 3:07 AM, Josh Cartwright <jo...@ni.com> wrote:
> > > >> > On Tue, Oct 27, 2015 at 05:09:12PM -0500, 
> > > >> > at...@opensource.altera.com wrote:
> > > >> >> From: Alan Tull <at...@opensource.altera.com>
> > > >> >>
> > > >> >> The Simple FPGA bus uses the FPGA Manager Framework and the
> > > >> >> FPGA Bridge Framework to provide a manufactorer-agnostic
> > > >> >> interface for reprogramming FPGAs that is Device Tree
> > > >> >> Overlays-based.
> > > >> >
> > > >> > Do you intend the "simple-fpga-bus" to be used on Zynq as well?  The
> > > >> > whole concept of the socfpga's "FPGA Bridge" doesn't map to the Zynq 
> > > >> > at
> > > >> > all, from what I can tell.
> > > >>
> > > >> For Zynq the zynq-fpga driver takes care of the level shifters on full
> > > >> reconfiguration,
> > > >> and doesn't for partial reconfiguration. Now depending on which parts
> > > >> of the fabric
> > > >> are partial reconfigured (say AXI masters), one might run into issues
> > > >> with a setup like that.
> > > >>
> > > >> My first plan was to counter that by using zynq-reset to hold the
> > > >> reset high during
> > > >> reconfiguration of that part of the FPGA.
> > > >>
> > > >> I'm happy to rethink that part and maybe redo the level shifters and
> > > >> resets together in a bridge
> > > >> driver under devicetree control gives finer grained control.
> > > >
> > > > There is already a framework which is used to describe and manipulate
> > > > level shifting/other IO properties, and that is pinctrl, and if we
> > > > wanted to use an appropriate abstraction, I think pinctrl would be the
> > > > best bet.
> > > 
> > > Alright, I'll investigate that. Again, for the non-partial reconfig
> > > case I'm happy
> > > with the behavior as implemented, for the partial reconfig I just
> > > haven't run into
> > > issues with not dealing with the level shifters.
> > 
> > Are you suggesting pinctrl instead of introducing a FPGA Bridge Framework?
> 
> I'm suggesting that for the set of operations/configuration states that
> need to be managed _for the Zynq[1]_ during reprogramming, I think
> pinctrl might be a good fit.
> 
> But the pinctrl state activation would happen in the context of the zynq
> fpga_mgr_ops write

*grr*... in the context of the zynq's write_init(), and write_complete()
callbacks.

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

Re: [PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

On Mon, Oct 26, 2015 at 05:44:22PM -0700, Paul E. McKenney wrote:
> On Mon, Oct 26, 2015 at 02:14:55PM -0500, Josh Cartwright wrote:
> > This reverts commit be3fc413da9eb17cce0991f214ab019d16c88c41.
> > 
> > While the use of synchronize_rcu_expedited() might make
> > synchronize_net() "faster", it does so at significant cost on RT
> > systems, as expediting a grace period forcibly preempts any
> > high-priority RT tasks (via the stop_machine() mechanism).
> > 
> > Without be3fc413da9e reverted, we can observe a latency spike up to 30us
> > with cyclictest by rapidly unplugging/reestablishing an ethernet link.
[..]
> 
> Hmmm...  If I remember correctly, using expedited here resulted
> in impressive performance improvements in some important cases.
> Perhaps things have changed (I must defer to Eric), but if not, how
> about something like this instead?
> 
>   if (rtnl_is_locked() && !IS_ENABLED(CONFIG_PREMPT_RT_FULL))
>   synchronize_rcu_expedited();
>   else
>   synchronize_rcu();
> 
> Alternatively, a boot-time option could be used:
> 
>   if (rtnl_is_locked() && !some_rt_boot_parameter)
>   synchronize_rcu_expedited();
>   else
>   synchronize_rcu();
> 
> I believe that the first alternative is better because it does the right
> thing without user intervention.  The second would be preferred should
> distros want to offer full RT by default, but I am guessing thta most
> distros would be reluctant to do this for some time to come.
> 
> Either way, these approaches have the advantage of giving RT users the
> latency they need, even in the face of networking configuration changes,
> while giving non-RT users the required performance of the networking
> configuration changes themselves.

Okay, yes, I like the first suggestion better as well, I've included a
patch below that does just that.  I hope you don't mind me turning it
into a Suggested-by :).

Thanks for taking a look!
  Josh

-- 8< --
From: Josh Cartwright 
Subject: [PATCH] net: make synchronize_rcu_expedited() conditional on RT_FULL

While the use of synchronize_rcu_expedited() might make
synchronize_net() "faster", it does so at significant cost on RT
systems, as expediting a grace period forcibly preempts any
high-priority RT tasks (via the stop_machine() mechanism).

Without this change, we can observe a latency spike up to 30us with
cyclictest by rapidly unplugging/reestablishing an ethernet link.

Cc: Eric Dumazet 
Cc: Paul E. McKenney 
Cc: David S. Miller 
Suggested-by: Paul E. McKenney 
Signed-off-by: Josh Cartwright 
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f8c23de..16fbef8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6969,7 +6969,7 @@ EXPORT_SYMBOL(free_netdev);
 void synchronize_net(void)
 {
might_sleep();
-   if (rtnl_is_locked())
+   if (rtnl_is_locked() && !IS_ENABLED(CONFIG_PREEMPT_RT_FULL))
synchronize_rcu_expedited();
else
synchronize_rcu();
-- 
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

On Mon, Oct 26, 2015 at 05:44:22PM -0700, Paul E. McKenney wrote:
> On Mon, Oct 26, 2015 at 02:14:55PM -0500, Josh Cartwright wrote:
> > This reverts commit be3fc413da9eb17cce0991f214ab019d16c88c41.
> > 
> > While the use of synchronize_rcu_expedited() might make
> > synchronize_net() "faster", it does so at significant cost on RT
> > systems, as expediting a grace period forcibly preempts any
> > high-priority RT tasks (via the stop_machine() mechanism).
> > 
> > Without be3fc413da9e reverted, we can observe a latency spike up to 30us
> > with cyclictest by rapidly unplugging/reestablishing an ethernet link.
[..]
> 
> Hmmm...  If I remember correctly, using expedited here resulted
> in impressive performance improvements in some important cases.
> Perhaps things have changed (I must defer to Eric), but if not, how
> about something like this instead?
> 
>   if (rtnl_is_locked() && !IS_ENABLED(CONFIG_PREMPT_RT_FULL))
>   synchronize_rcu_expedited();
>   else
>   synchronize_rcu();
> 
> Alternatively, a boot-time option could be used:
> 
>   if (rtnl_is_locked() && !some_rt_boot_parameter)
>   synchronize_rcu_expedited();
>   else
>   synchronize_rcu();
> 
> I believe that the first alternative is better because it does the right
> thing without user intervention.  The second would be preferred should
> distros want to offer full RT by default, but I am guessing thta most
> distros would be reluctant to do this for some time to come.
> 
> Either way, these approaches have the advantage of giving RT users the
> latency they need, even in the face of networking configuration changes,
> while giving non-RT users the required performance of the networking
> configuration changes themselves.

Okay, yes, I like the first suggestion better as well, I've included a
patch below that does just that.  I hope you don't mind me turning it
into a Suggested-by :).

Thanks for taking a look!
  Josh

-- 8< --
From: Josh Cartwright <jo...@ni.com>
Subject: [PATCH] net: make synchronize_rcu_expedited() conditional on RT_FULL

While the use of synchronize_rcu_expedited() might make
synchronize_net() "faster", it does so at significant cost on RT
systems, as expediting a grace period forcibly preempts any
high-priority RT tasks (via the stop_machine() mechanism).

Without this change, we can observe a latency spike up to 30us with
cyclictest by rapidly unplugging/reestablishing an ethernet link.

Cc: Eric Dumazet <eric.duma...@gmail.com>
Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com>
Cc: David S. Miller <da...@davemloft.net>
Suggested-by: Paul E. McKenney <paul...@linux.vnet.ibm.com>
Signed-off-by: Josh Cartwright <jo...@ni.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f8c23de..16fbef8 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6969,7 +6969,7 @@ EXPORT_SYMBOL(free_netdev);
 void synchronize_net(void)
 {
might_sleep();
-   if (rtnl_is_locked())
+   if (rtnl_is_locked() && !IS_ENABLED(CONFIG_PREEMPT_RT_FULL))
synchronize_rcu_expedited();
else
synchronize_rcu();
-- 
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

This reverts commit be3fc413da9eb17cce0991f214ab019d16c88c41.

While the use of synchronize_rcu_expedited() might make
synchronize_net() "faster", it does so at significant cost on RT
systems, as expediting a grace period forcibly preempts any
high-priority RT tasks (via the stop_machine() mechanism).

Without be3fc413da9e reverted, we can observe a latency spike up to 30us
with cyclictest by rapidly unplugging/reestablishing an ethernet link.

Cc: Eric Dumazet 
Cc: Paul E. McKenney 
Cc: David S. Miller 
Signed-off-by: Josh Cartwright 
---
 net/core/dev.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f8c23de..869ef62 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6969,10 +6969,7 @@ EXPORT_SYMBOL(free_netdev);
 void synchronize_net(void)
 {
might_sleep();
-   if (rtnl_is_locked())
-   synchronize_rcu_expedited();
-   else
-   synchronize_rcu();
+   synchronize_rcu();
 }
 EXPORT_SYMBOL(synchronize_net);
 
-- 
2.5.1

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

[PATCH -rt] Revert "net: use synchronize_rcu_expedited()"

[Josh Cartwright]

This reverts commit be3fc413da9eb17cce0991f214ab019d16c88c41.

While the use of synchronize_rcu_expedited() might make
synchronize_net() "faster", it does so at significant cost on RT
systems, as expediting a grace period forcibly preempts any
high-priority RT tasks (via the stop_machine() mechanism).

Without be3fc413da9e reverted, we can observe a latency spike up to 30us
with cyclictest by rapidly unplugging/reestablishing an ethernet link.

Cc: Eric Dumazet <eric.duma...@gmail.com>
Cc: Paul E. McKenney <paul...@linux.vnet.ibm.com>
Cc: David S. Miller <da...@davemloft.net>
Signed-off-by: Josh Cartwright <jo...@ni.com>
---
 net/core/dev.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index f8c23de..869ef62 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6969,10 +6969,7 @@ EXPORT_SYMBOL(free_netdev);
 void synchronize_net(void)
 {
might_sleep();
-   if (rtnl_is_locked())
-   synchronize_rcu_expedited();
-   else
-   synchronize_rcu();
+   synchronize_rcu();
 }
 EXPORT_SYMBOL(synchronize_net);
 
-- 
2.5.1

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

Re: [RFC] fpga: socfpga: Fix check of return value of devm_request_irq

[Josh Cartwright]

On Thu, Oct 22, 2015 at 02:24:03PM -0500, atull wrote:
> > The return value should be checked for non-zero, instead
> > of checking it being IS_ERR_VALUE().
> > 
> > Signed-off-by: Moritz Fischer 
> On Thu, 22 Oct 2015, Moritz Fischer wrote:
> 
> Hi Moritz,
> 
> Thank you, yes this is better.
> 
> I don't know if I need to ack everything, but here it is
> anyway:
> 
> Acked-by: Alan Tull 

Because Greg is the one picking up patches for the FPGA manager stuff
right now, you'll want to make sure he's at least CC'd.  A proper resend
would likely be the easiest way for him to pick it up.

Feel free to add by Reviewed-by as well:

Reviewed-by: Josh Cartwright 

Thanks,
  Josh

> > ---
> >  drivers/fpga/socfpga.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> > index 706b80d..27d2ff2 100644
> > --- a/drivers/fpga/socfpga.c
> > +++ b/drivers/fpga/socfpga.c
> > @@ -577,7 +577,7 @@ static int socfpga_fpga_probe(struct platform_device 
> > *pdev)
> >  
> > ret = devm_request_irq(dev, priv->irq, socfpga_fpga_isr, 0,
> >dev_name(dev), priv);
> > -   if (IS_ERR_VALUE(ret))
> > +   if (ret)
> > return ret;
> >  
> > return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> > -- 
> > 2.4.3
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


signature.asc
Description: PGP signature

Re: [RFC] fpga: socfpga: Fix check of return value of devm_request_irq

[Josh Cartwright]

On Thu, Oct 22, 2015 at 02:24:03PM -0500, atull wrote:
> > The return value should be checked for non-zero, instead
> > of checking it being IS_ERR_VALUE().
> > 
> > Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>
> On Thu, 22 Oct 2015, Moritz Fischer wrote:
> 
> Hi Moritz,
> 
> Thank you, yes this is better.
> 
> I don't know if I need to ack everything, but here it is
> anyway:
> 
> Acked-by: Alan Tull <at...@opensource.altera.com>

Because Greg is the one picking up patches for the FPGA manager stuff
right now, you'll want to make sure he's at least CC'd.  A proper resend
would likely be the easiest way for him to pick it up.

Feel free to add by Reviewed-by as well:

Reviewed-by: Josh Cartwright <jo...@eso.teric.us>

Thanks,
  Josh

> > ---
> >  drivers/fpga/socfpga.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> > index 706b80d..27d2ff2 100644
> > --- a/drivers/fpga/socfpga.c
> > +++ b/drivers/fpga/socfpga.c
> > @@ -577,7 +577,7 @@ static int socfpga_fpga_probe(struct platform_device 
> > *pdev)
> >  
> > ret = devm_request_irq(dev, priv->irq, socfpga_fpga_isr, 0,
> >dev_name(dev), priv);
> > -   if (IS_ERR_VALUE(ret))
> > +   if (ret)
> > return ret;
> >  
> > return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
> > -- 
> > 2.4.3
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


signature.asc
Description: PGP signature

Re: [PATCHv2] fpga: zynq-fpga: Change fw format to handle bin instead of bit.

[Josh Cartwright]

On Tue, Oct 20, 2015 at 10:19:56AM -0700, Moritz Fischer wrote:
> This gets rid of the code to strip away the header and byteswap,
> as well as the check for the sync word.
> 
> Signed-off-by: Moritz Fischer 

Simpler is better.

Reviewed-by: Josh Cartwright 

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

Re: [PATCHv2] fpga: zynq-fpga: Change fw format to handle bin instead of bit.

[Josh Cartwright]

On Tue, Oct 20, 2015 at 10:19:56AM -0700, Moritz Fischer wrote:
> This gets rid of the code to strip away the header and byteswap,
> as well as the check for the sync word.
> 
> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>

Simpler is better.

Reviewed-by: Josh Cartwright <jo...@ni.com>

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

Re: [PATCHv2 2/3] ARM: zynq: dt: Updated devicetree for Zynq 7000 platform.

[Josh Cartwright]

On Mon, Oct 19, 2015 at 04:09:09PM +0200, Michal Simek wrote:
> On 10/18/2015 07:53 PM, Josh Cartwright wrote:
> > On Fri, Oct 16, 2015 at 03:42:29PM -0700, Moritz Fischer wrote:
[..]
> >> @@ -294,6 +294,11 @@
> >>devcfg: devcfg@f8007000 {
> >>compatible = "xlnx,zynq-devcfg-1.0";
> >>reg = <0xf8007000 0x100>;
> >> +  interrupt-parent = <>;
> > 
> > You shouldn't need interrupt-parent here.  In fact, I suspect it can be
> > removed from all sibling nodes as well.
> 
> Correct. But I tend to do it vice-versa. To remove it from amba node and
> keep it in every IP here. The reason is simple to let everybody know
> that setting up right interrupt controller is something what they have
> to care. If you have more interrupt controllers in the system it can be
> messy.

If you like that, then you'd like the interrupt-extended properly even
better (I think).

> But again. Please remove this patch from this series. This will go to
> mainline through arm-soc but 1/3 and 3/3 will go through Greg. That's
> why please do not include it here.

Not all of the patches in a series need to go through the same
maintainer...splitting them up is common.  It's nice seeing related
things out on the list together.  (Especially when there is a dependency
at some level).

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

Re: [PATCHv2 2/3] ARM: zynq: dt: Updated devicetree for Zynq 7000 platform.

[Josh Cartwright]

On Mon, Oct 19, 2015 at 04:09:09PM +0200, Michal Simek wrote:
> On 10/18/2015 07:53 PM, Josh Cartwright wrote:
> > On Fri, Oct 16, 2015 at 03:42:29PM -0700, Moritz Fischer wrote:
[..]
> >> @@ -294,6 +294,11 @@
> >>devcfg: devcfg@f8007000 {
> >>compatible = "xlnx,zynq-devcfg-1.0";
> >>reg = <0xf8007000 0x100>;
> >> +  interrupt-parent = <>;
> > 
> > You shouldn't need interrupt-parent here.  In fact, I suspect it can be
> > removed from all sibling nodes as well.
> 
> Correct. But I tend to do it vice-versa. To remove it from amba node and
> keep it in every IP here. The reason is simple to let everybody know
> that setting up right interrupt controller is something what they have
> to care. If you have more interrupt controllers in the system it can be
> messy.

If you like that, then you'd like the interrupt-extended properly even
better (I think).

> But again. Please remove this patch from this series. This will go to
> mainline through arm-soc but 1/3 and 3/3 will go through Greg. That's
> why please do not include it here.

Not all of the patches in a series need to go through the same
maintainer...splitting them up is common.  It's nice seeing related
things out on the list together.  (Especially when there is a dependency
at some level).

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

Re: [PATCHv2 0/3] Adding FPGA Manager support for Xilinx Zynq

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:27PM -0700, Moritz Fischer wrote:
> Hi all,
> 
> I've tried to address most of the feedback that was brought up,
> the one thing I haven't looked at was the firmware format part,
> since that was still in discussion.
> So I'm still open to suggestions on how to handle this.

Was there disagreement?  I had thought we settled on limiting the
handling the BIN format explicitly, the rest of the thread was about
userspace tooling for BIT->BIN conversion.

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

Re: [PATCHv2 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 16, 2015 at 03:42:30PM -0700, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code borrows some from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
> ---
> 
> v2:
>  - Replaced locking error flag and broken completion with irq masking
>and changed completion handling
>  - Dealing with timeout cases
>  - Reworked clock handling
>  - Moved initialization from probe() to write_init()
>  - Fixed return value of devm_request_irq() check to check for non-zero
>  - Alphabetized includes ;-)
>  - Changed some of the comments, to better explain what's happening
[..]
> +static int zynq_fpga_probe(struct platform_device *pdev)
> +{
[..]
> + priv->clk = devm_clk_get(dev, "ref_clk");
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "input clock not found");
> + return PTR_ERR(priv->clk);
> + }
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err) {
> + dev_err(dev, "unable to enable clock");
> + return err;
> + }

prepare_cnt = 1, enable_cnt = 1

> +
> + /* unlock the device */
> + zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
> +
> + clk_disable(priv->clk);

prepare_cnt = 1, enable_cnt = 0
> +
> + err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> + _fpga_ops, priv);
> + if (err) {
> + dev_err(dev, "unable to register FPGA manager");
> + clk_disable_unprepare(priv->clk);

prepare_cnt = 0, enable_cnt = -1 /* OOPS! */

Clock management is still wonky.  I think you only want clk_unprepare here.

> + return err;
> + }
> +

Assuming all goes well, you'll be leaving probe() with:

prepare_cnt = 1, enable_cnt = 0.

> + return 0;
> +}
> +
> +static int zynq_fpga_remove(struct platform_device *pdev)
> +{
> + struct zynq_fpga_priv *priv;
> +
> + fpga_mgr_unregister(>dev);
> +
> + priv = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(priv->clk);

Which means, symmetrically, you'll only want this to be a clk_unprepare().

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

Re: [PATCHv2 2/3] ARM: zynq: dt: Updated devicetree for Zynq 7000 platform.

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:29PM -0700, Moritz Fischer wrote:
> Added addtional nodes required for FPGA Manager operation
> of the Xilinx Zynq Devc configuration interface.
> 
> Reviewed-by: Sören Brinkmann 
> Signed-off-by: Moritz Fischer 
> ---
> 
> v2: No changes
> 
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi 
> b/arch/arm/boot/dts/zynq-7000.dtsi
> index dc0457e..1a5220e 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -294,6 +294,11 @@
>   devcfg: devcfg@f8007000 {
>   compatible = "xlnx,zynq-devcfg-1.0";
>   reg = <0xf8007000 0x100>;
> + interrupt-parent = <>;

You shouldn't need interrupt-parent here.  In fact, I suspect it can be
removed from all sibling nodes as well.

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

Re: [PATCHv2 1/3] ARM: dt: fpga: Added binding docs for Xilinx Zynq FPGA manager.

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:28PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer 
> ---
> 
> v2:
>  - Clock names are now a required property
>  - Removed interrupt-parent property
> 
> ---
>  .../devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt | 19 
> +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 000..7018aa8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,19 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "xlnx,zynq-devcfg-1.0"
> +- reg:   base address and size for memory mapped io
> +- interrupts:interrupt for the FPGA manager device
> +- clocks:phandle for clocks required operation

Technically a "clock specifier", but other than that:

Reviewed-by: Josh Cartwright 

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

Re: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Josh Cartwright]

Hello,

I've got a few comments below.

On Sat, Oct 17, 2015 at 12:52:18PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada 
> Signed-off-by: Ravi Kiran Gummaluri 
> ---
> Added MSI domain implementation for handling MSI interrupts
> Added implementation for chained irq handlers
> Added legacy domain for handling legacy interrupts
> Added child node for handling legacy interrupt
[..]
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[..]
> +static inline bool nwl_pcie_link_up(struct nwl_pcie *pcie, u32 check_link_up)
> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_link_up == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_link_up == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;

It would seem marginally simpler if the the bit to check was passed as
the argument, instead of a check_link_up -> bit mapping.

> +}
> +
> +/**
> + * nwl_pcie_get_config_base - Get configuration base
> + *
> + * @bus: Bus structure of current bus
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + *  accessed.
> + */
> +static void __iomem *nwl_pcie_get_config_base(struct pci_bus *bus,
> +  unsigned int devfn,
> +  int where)
> +{
> + struct nwl_pcie *pcie = bus->sysdata;
> + int relbus;
> +
> + relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> + (devfn << ECAM_DEV_LOC_SHIFT);
> +
> + return pcie->ecam_base + relbus + where;
> +}

It would seem you could simplify if you provide this as your map_bus
implementation of pci_ops.  Then you could use the
pci_generic_config_read/_write callbacks.

> +/**
> + * nwl_setup_sspl - Set Slot Power limit
> + *
> + * @pcie: PCIe port information
> + */
> +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> +{
> + unsigned int status;
> + int retval = 0;
> +
> + do {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG) & MSG_BUSY_BIT;
> + if (!status) {
> + /*
> +  * Generate the TLP message for a single EP
> +  * [TODO] Add a multi-endpoint code
> +  */
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_LO);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + /* Pattern to generate SSLP TLP */
> + nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +   TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> + status = 0;

This assignment looks useless?

> + mdelay(2);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_BIT;
> + if (status) {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_STATUS_BIT;
> + if (status == SLVERR) {
> + dev_err(pcie->dev, "AXI slave error");
> + retval = SLVERR;
> + } else if (status == DECERR) {
> + dev_err(pcie->dev, "AXI Decode error");
> + retval = DECERR;
> + }
> + } else {
> + retval = 1;
> + }
> + }
> + } while (status);
> +
> + return retval;
> +}
> +
[..]
> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> + int where,
> + int size,
> + u32 val)
> +{
> + void __iomem *addr;
> + int retval;
> + struct nwl_pcie *pcie = bus->sysdata;
> +
> + if (!bus->number && devfn > 0)
> + 

Re: [PATCHv2 0/3] Adding FPGA Manager support for Xilinx Zynq

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:27PM -0700, Moritz Fischer wrote:
> Hi all,
> 
> I've tried to address most of the feedback that was brought up,
> the one thing I haven't looked at was the firmware format part,
> since that was still in discussion.
> So I'm still open to suggestions on how to handle this.

Was there disagreement?  I had thought we settled on limiting the
handling the BIN format explicitly, the rest of the thread was about
userspace tooling for BIT->BIN conversion.

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

Re: [PATCH v4] PCI: Xilinx-NWL-PCIe: Added support for Xilinx NWL PCIe Host Controller

[Josh Cartwright]

Hello,

I've got a few comments below.

On Sat, Oct 17, 2015 at 12:52:18PM +0530, Bharat Kumar Gogada wrote:
> Adding PCIe Root Port driver for Xilinx PCIe NWL bridge IP.
> 
> Signed-off-by: Bharat Kumar Gogada 
> Signed-off-by: Ravi Kiran Gummaluri 
> ---
> Added MSI domain implementation for handling MSI interrupts
> Added implementation for chained irq handlers
> Added legacy domain for handling legacy interrupts
> Added child node for handling legacy interrupt
[..]
> +++ b/drivers/pci/host/pcie-xilinx-nwl.c
[..]
> +static inline bool nwl_pcie_link_up(struct nwl_pcie *pcie, u32 check_link_up)
> +{
> + unsigned int status = -EINVAL;
> +
> + if (check_link_up == PCIE_USER_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PCIE_PHY_LINKUP_BIT) ? 1 : 0;
> + else if (check_link_up == PHY_RDY_LINKUP)
> + status = (readl(pcie->pcireg_base + PS_LINKUP_OFFSET) &
> +   PHY_RDY_LINKUP_BIT) ? 1 : 0;
> + return status;

It would seem marginally simpler if the the bit to check was passed as
the argument, instead of a check_link_up -> bit mapping.

> +}
> +
> +/**
> + * nwl_pcie_get_config_base - Get configuration base
> + *
> + * @bus: Bus structure of current bus
> + * @devfn: Device/function
> + * @where: Offset from base
> + *
> + * Return: Base address of the configuration space needed to be
> + *  accessed.
> + */
> +static void __iomem *nwl_pcie_get_config_base(struct pci_bus *bus,
> +  unsigned int devfn,
> +  int where)
> +{
> + struct nwl_pcie *pcie = bus->sysdata;
> + int relbus;
> +
> + relbus = (bus->number << ECAM_BUS_LOC_SHIFT) |
> + (devfn << ECAM_DEV_LOC_SHIFT);
> +
> + return pcie->ecam_base + relbus + where;
> +}

It would seem you could simplify if you provide this as your map_bus
implementation of pci_ops.  Then you could use the
pci_generic_config_read/_write callbacks.

> +/**
> + * nwl_setup_sspl - Set Slot Power limit
> + *
> + * @pcie: PCIe port information
> + */
> +static int nwl_setup_sspl(struct nwl_pcie *pcie)
> +{
> + unsigned int status;
> + int retval = 0;
> +
> + do {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG) & MSG_BUSY_BIT;
> + if (!status) {
> + /*
> +  * Generate the TLP message for a single EP
> +  * [TODO] Add a multi-endpoint code
> +  */
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_LO);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_SPEC_HI);
> + nwl_bridge_writel(pcie, 0x0,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + /* Pattern to generate SSLP TLP */
> + nwl_bridge_writel(pcie, PATTRN_SSLP_TLP,
> +   TX_PCIE_MSG + TX_PCIE_MSG_CNTL);
> + nwl_bridge_writel(pcie, RANDOM_DIGIT,
> +   TX_PCIE_MSG + TX_PCIE_MSG_DATA);
> + nwl_bridge_writel(pcie, nwl_bridge_readl(pcie,
> +   TX_PCIE_MSG) | 0x1, TX_PCIE_MSG);
> + status = 0;

This assignment looks useless?

> + mdelay(2);
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_BIT;
> + if (status) {
> + status = nwl_bridge_readl(pcie, TX_PCIE_MSG)
> +   & MSG_DONE_STATUS_BIT;
> + if (status == SLVERR) {
> + dev_err(pcie->dev, "AXI slave error");
> + retval = SLVERR;
> + } else if (status == DECERR) {
> + dev_err(pcie->dev, "AXI Decode error");
> + retval = DECERR;
> + }
> + } else {
> + retval = 1;
> + }
> + }
> + } while (status);
> +
> + return retval;
> +}
> +
[..]
> +static int nwl_nwl_writel_config(struct pci_bus *bus,
> + unsigned int devfn,
> + int where,
> + int size,
> + u32 val)
> +{
> + void __iomem *addr;
> + int retval;
> + struct nwl_pcie *pcie = bus->sysdata;
> +
> + if 

Re: [PATCHv2 2/3] ARM: zynq: dt: Updated devicetree for Zynq 7000 platform.

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:29PM -0700, Moritz Fischer wrote:
> Added addtional nodes required for FPGA Manager operation
> of the Xilinx Zynq Devc configuration interface.
> 
> Reviewed-by: Sören Brinkmann 
> Signed-off-by: Moritz Fischer 
> ---
> 
> v2: No changes
> 
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/zynq-7000.dtsi 
> b/arch/arm/boot/dts/zynq-7000.dtsi
> index dc0457e..1a5220e 100644
> --- a/arch/arm/boot/dts/zynq-7000.dtsi
> +++ b/arch/arm/boot/dts/zynq-7000.dtsi
> @@ -294,6 +294,11 @@
>   devcfg: devcfg@f8007000 {
>   compatible = "xlnx,zynq-devcfg-1.0";
>   reg = <0xf8007000 0x100>;
> + interrupt-parent = <>;

You shouldn't need interrupt-parent here.  In fact, I suspect it can be
removed from all sibling nodes as well.

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

Re: [PATCHv2 1/3] ARM: dt: fpga: Added binding docs for Xilinx Zynq FPGA manager.

[Josh Cartwright]

On Fri, Oct 16, 2015 at 03:42:28PM -0700, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer <moritz.fisc...@ettus.com>
> ---
> 
> v2:
>  - Clock names are now a required property
>  - Removed interrupt-parent property
> 
> ---
>  .../devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt | 19 
> +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 000..7018aa8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,19 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "xlnx,zynq-devcfg-1.0"
> +- reg:   base address and size for memory mapped io
> +- interrupts:interrupt for the FPGA manager device
> +- clocks:phandle for clocks required operation

Technically a "clock specifier", but other than that:

Reviewed-by: Josh Cartwright <jo...@ni.com>

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

Re: [PATCHv2 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 16, 2015 at 03:42:30PM -0700, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code borrows some from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
> ---
> 
> v2:
>  - Replaced locking error flag and broken completion with irq masking
>and changed completion handling
>  - Dealing with timeout cases
>  - Reworked clock handling
>  - Moved initialization from probe() to write_init()
>  - Fixed return value of devm_request_irq() check to check for non-zero
>  - Alphabetized includes ;-)
>  - Changed some of the comments, to better explain what's happening
[..]
> +static int zynq_fpga_probe(struct platform_device *pdev)
> +{
[..]
> + priv->clk = devm_clk_get(dev, "ref_clk");
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "input clock not found");
> + return PTR_ERR(priv->clk);
> + }
> +
> + err = clk_prepare_enable(priv->clk);
> + if (err) {
> + dev_err(dev, "unable to enable clock");
> + return err;
> + }

prepare_cnt = 1, enable_cnt = 1

> +
> + /* unlock the device */
> + zynq_fpga_write(priv, UNLOCK_OFFSET, UNLOCK_MASK);
> +
> + clk_disable(priv->clk);

prepare_cnt = 1, enable_cnt = 0
> +
> + err = fpga_mgr_register(dev, "Xilinx Zynq FPGA Manager",
> + _fpga_ops, priv);
> + if (err) {
> + dev_err(dev, "unable to register FPGA manager");
> + clk_disable_unprepare(priv->clk);

prepare_cnt = 0, enable_cnt = -1 /* OOPS! */

Clock management is still wonky.  I think you only want clk_unprepare here.

> + return err;
> + }
> +

Assuming all goes well, you'll be leaving probe() with:

prepare_cnt = 1, enable_cnt = 0.

> + return 0;
> +}
> +
> +static int zynq_fpga_remove(struct platform_device *pdev)
> +{
> + struct zynq_fpga_priv *priv;
> +
> + fpga_mgr_unregister(>dev);
> +
> + priv = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(priv->clk);

Which means, symmetrically, you'll only want this to be a clk_unprepare().

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

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Josh Cartwright]

On Wed, Oct 07, 2015 at 03:33:50AM +, Anup Patel wrote:
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> > On 06/10/15 15:25, Scott Branden wrote:
[..]
> > Then instead of adding a "reset flag" to Device Tree, another approach 
> > could be
> > to put the desired or currently configured exhaustive list of NAND timings 
> > in
> > Device Tree, and based on that you could have this:
> > 
> > - the NAND controller driver finds that these timings match the current
> > configuration, you are good to go
> > 
> > - the NAND controller drivers finds a difference in how current timings are
> > configured vs. desired timings, and issues a controller reset, prior to 
> > applying
> > new timing configuration
>
> To add to this ...
>
> The mechanism to reset is BRCM NAND controller is SOC specific so the
> SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> to reset the NAND controller.
>
> For iProc SoC family, the NAND controller reset is through IDM register
> space which is only iomap'ed by iproc_nand.c.
>
> We might end-up having one more SoC specific callback which will be
> Provided by iproc_nand.c to brcmnand.c.

Not that I'm familiar with these SoCs, but I did want to chime in and
make sure you are aware of the existing reset_controller_dev
abstraction, which is intended to solve exactly this problem.  Including
a reset_control_get_optional() that might fit your use case.  See
include/linux/reset{,-controller}.h.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

[Josh Cartwright]

On Wed, Oct 07, 2015 at 03:33:50AM +, Anup Patel wrote:
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> > On 06/10/15 15:25, Scott Branden wrote:
[..]
> > Then instead of adding a "reset flag" to Device Tree, another approach 
> > could be
> > to put the desired or currently configured exhaustive list of NAND timings 
> > in
> > Device Tree, and based on that you could have this:
> > 
> > - the NAND controller driver finds that these timings match the current
> > configuration, you are good to go
> > 
> > - the NAND controller drivers finds a difference in how current timings are
> > configured vs. desired timings, and issues a controller reset, prior to 
> > applying
> > new timing configuration
>
> To add to this ...
>
> The mechanism to reset is BRCM NAND controller is SOC specific so the
> SoC independent BRCM NAND driver (i.e. brcmnand.c) does not know how
> to reset the NAND controller.
>
> For iProc SoC family, the NAND controller reset is through IDM register
> space which is only iomap'ed by iproc_nand.c.
>
> We might end-up having one more SoC specific callback which will be
> Provided by iproc_nand.c to brcmnand.c.

Not that I'm familiar with these SoCs, but I did want to chime in and
make sure you are aware of the existing reset_controller_dev
abstraction, which is intended to solve exactly this problem.  Including
a reset_control_get_optional() that might fit your use case.  See
include/linux/reset{,-controller}.h.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> + u32 intr_status;
> + struct zynq_fpga_priv *priv = data;
> +
> + spin_lock(>lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> + if (!intr_status) {
> + spin_unlock(>lock);
> + return IRQ_NONE;
> + }
> +
> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> + complete(>dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> + IXR_ERROR_FLAGS_MASK) {
> + priv->error = true;
> + dev_err(priv->dev, "%s dma error\n", __func__);
> + }
> + spin_unlock(>lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;
> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> + }
> +
> + clk_disable(priv->clk);
> +
> + return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
> + }

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> + /* remove the header, align the data on 

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Josh Cartwright]

On Fri, Oct 09, 2015 at 12:45:05AM +0200, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer 
> ---
>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt | 26 
> ++
>  1 file changed, 26 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 000..82ffda8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,26 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "xlnx,zynq-devcfg-1.0"
> +- reg:   base address and size for memory mapped io
> +- interrupt parent:  interrupt source phandle

I think you mean 'interrupt-parent', with the hyphen.

Actually, this isn't really a 'required' property of this node, as it
could be specified in a parent node.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 3/3] fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000

[Josh Cartwright]

Hey Moritz-

On Fri, Oct 09, 2015 at 12:45:07AM +0200, Moritz Fischer wrote:
> This commit adds FPGA Manager support for the Xilinx Zynq chip.
> The code heavily borrows from the xdevcfg driver in Xilinx'
> vendor tree.
> 
> Signed-off-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/zynq-fpga.c
[..]
> +static irqreturn_t zynq_fpga_isr(int irq, void *data)
> +{
> + u32 intr_status;
> + struct zynq_fpga_priv *priv = data;
> +
> + spin_lock(>lock);

I'm confused about the locking here.  You have this lock, but it's only
used in the isr.  It's claimed purpose is to protect 'error', but that
clearly isn't happening.

> + intr_status = zynq_fpga_read(priv, INT_STS_OFFSET);
> +
> + if (!intr_status) {
> + spin_unlock(>lock);
> + return IRQ_NONE;
> + }
> +
> + zynq_fpga_write(priv, INT_STS_OFFSET, intr_status);
> +
> + if ((intr_status & IXR_D_P_DONE_MASK) == IXR_D_P_DONE_MASK)
> + complete(>dma_done);

Just so I understand, wouldn't you also want to complete() in the error
case, too?

> + if ((intr_status & IXR_ERROR_FLAGS_MASK) ==
> + IXR_ERROR_FLAGS_MASK) {
> + priv->error = true;
> + dev_err(priv->dev, "%s dma error\n", __func__);
> + }
> + spin_unlock(>lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int zynq_fpga_ops_write_init(struct fpga_manager *mgr, u32 flags,
> + const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + u32 ctrl, status;
> + int err;
> +
> + priv = mgr->priv;
> +
> + err = clk_enable(priv->clk);
> + if (err)
> + return err;
> +
> + /* only reset if we're not doing partial reconfig */
> + if (!(flags & FPGA_MGR_PARTIAL_RECONFIG)) {
> + /* assert AXI interface resets */
> + regmap_write(priv->slcr, SLCR_FPGA_RST_CTRL_OFFSET,
> +  FPGA_RST_ALL_MASK);
> +
> + /* disable level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_DISABLE_ALL_MASK);
> + /* enable output level shifters */
> + regmap_write(priv->slcr, SLCR_LVL_SHFTR_EN_OFFSET,
> +  LVL_SHFTR_ENABLE_PS_TO_PL);
> +
> + /* create a rising edge on PCFG_INIT. PCFG_INIT follows
> +  * PCFG_PROG_B, so we need to poll it after setting PCFG_PROG_B
> +  * to make sure the rising edge actually happens
> +  */
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl &= ~CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, !(status &
> +STATUS_PCFG_INIT_MASK), 20, 0);

And if we timeout?

> +
> + ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> + ctrl |= CTRL_PCFG_PROG_B_MASK;
> +
> + zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
> +
> + zynq_fpga_poll_timeout(priv, STATUS_OFFSET, status, status &
> +STATUS_PCFG_INIT_MASK, 20, 0);

And if we timeout?

> + }
> +
> + clk_disable(priv->clk);
> +
> + return 0;
> +}
> +
> +static int zynq_fpga_ops_write(struct fpga_manager *mgr,
> +const char *buf, size_t count)
> +{
> + struct zynq_fpga_priv *priv;
> + int err;
> + char *kbuf;
> + size_t i, in_count;
> + dma_addr_t dma_addr;
> + u32 transfer_length = 0;
> + bool endian_swap = false;
> +
> + in_count = count;
> + priv = mgr->priv;
> +
> + kbuf = dma_alloc_coherent(priv->dev, count, _addr, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + memcpy(kbuf, buf, count);
> +
> + /* look for the sync word */
> + for (i = 0; i < count - 4; i++) {
> + if (memcmp(kbuf + i, "\x66\x55\x99\xAA", 4) == 0) {
> + dev_dbg(priv->dev, "Found normal sync word\n");
> + endian_swap = false;
> + break;
> + }
> + if (memcmp(kbuf + i, "\xAA\x99\x55\x66", 4) == 0) {
> + dev_dbg(priv->dev, "Found swapped sync word\n");
> + endian_swap = true;
> + break;
> + }
> + }

How much control do we have over mandating the format of firmware at
this point?  It'd be swell if we could just mandate a specific
endianness, and leave this munging to usermode.

> + /* remove the 

Re: [PATCH 1/3] doc: dt: fpga: Added Documentation for Xilinx Zynq FPGA manager.

[Josh Cartwright]

On Fri, Oct 09, 2015 at 12:45:05AM +0200, Moritz Fischer wrote:
> Signed-off-by: Moritz Fischer 
> ---
>  .../bindings/fpga/xilinx-zynq-fpga-mgr.txt | 26 
> ++
>  1 file changed, 26 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> 
> diff --git a/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt 
> b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> new file mode 100644
> index 000..82ffda8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fpga/xilinx-zynq-fpga-mgr.txt
> @@ -0,0 +1,26 @@
> +Xilinx Zynq FPGA Manager
> +
> +Required properties:
> +- compatible:should contain "xlnx,zynq-devcfg-1.0"
> +- reg:   base address and size for memory mapped io
> +- interrupt parent:  interrupt source phandle

I think you mean 'interrupt-parent', with the hyphen.

Actually, this isn't really a 'required' property of this node, as it
could be specified in a parent node.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] leds: add device activity LED triggers

[Josh Cartwright]

On Fri, Oct 02, 2015 at 09:45:37AM +0200, Maciek Borzecki wrote:
> On 10/01 09:47, Josh Cartwright wrote:
> > On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> > > The patch adds LED triggers for indicating an activity on a selected
> > > device. The drivers that intend to use triggers need to register
> > > respective devices using ledtrig_dev_add(). Triggers are generated by
> > > explicitly calling ledtrig_dev_activity().
> > >
> > > Signed-off-by: Maciek Borzecki 
> > > ---
> > [..]
> > > +struct ledtrig_dev_data {
> > > + char name[MAX_NAME_LEN];
> > > + dev_t dev;
> > > + struct led_trigger *trig;
> > > + struct list_head node;
> > > +};
> > > +
> > > +/**
> > > + * ledtrig_dev_activity - signal activity on device
> > > + * @dev: device
> > > + *
> > > + * Fires a trigger assigned to @dev device.
> > > + */
> > > +void ledtrig_dev_activity(dev_t dev)
> >
> > It seems a bit strange to me to associate a device LED trigger with
> > dev_t.  Some devices don't expose a dev node, some devices expose
> > multiple dev nodes...
> >
> > Is there a reason why you are not tying to the device model?
> >
> Thanks for the comments.
> 
> The first proof of concept used `sturct device` as parameter in all API
> calls, but then there's a problem of naming of a trigger in a sane
> way. The trigger name followed the same approach as __dev_printk, and
> the naming was done in this fashion:
> 
>sprintf(..., "dev-%s-%s", dev_driver_string(dev), dev_name(dev));
> 
> Then for instance on wandboard, /dev/ttymxc0 and /dev/ttymxc2 would
> appear as `dev-serial-202` and `dev-serial-21ec000`. In my opinion
> this was unnecessarily complicated.

Hmm, maybe we're bikeshedding at this point, but LEDs with those names
seem much more straightfoward to me than a "dev-:" name, for
devices which have done dynamic dev_t allocation.

> Also, if I'm not mistaken, using this approach the partitions on MMC
> card or SATA drive would end up with the same trigger name, as it is a
> single device.

This would only be true if you used _just_ the struct device.  I was
imagining that you'd specify a (struct device, unsigned index) pair.
Better, you could do a (struct device, const char *) pair.

Also, from a lifetime management perspective, it starts to feel like
something that might integrate better as a managed resource (devm_*).

[..]
> Multiple dev nodes will already have different minor numbers, so
> their dev_t is different anyway.

Okay, backing up I don't really see what this API really buys the
consumer.  The dev_t -> struct led_trigger mapping just seems like a
total waste.  Why not just make your ledtrig_dev_add() function return
the struct led_trigger * that the consumer keeps track of?

Maybe seeing an example consumer would provide some clarification.

> As for devices that do not have a dev_t assigned to them one can still
> pass a custom tag in ledtrig_dev_add(). It's just a number so as long as
> there's no collision in numbering things should be fine.

Ensuring no collision will be difficult, especially given that it's most
common that the dynamic allocator is used.  In order to guarantee no
collisions, a user who doesn't expose any device nodes would need to do
their own dev_t allocation...to use this interface.  And that seems
silly to me.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] leds: add device activity LED triggers

[Josh Cartwright]

On Fri, Oct 02, 2015 at 09:45:37AM +0200, Maciek Borzecki wrote:
> On 10/01 09:47, Josh Cartwright wrote:
> > On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> > > The patch adds LED triggers for indicating an activity on a selected
> > > device. The drivers that intend to use triggers need to register
> > > respective devices using ledtrig_dev_add(). Triggers are generated by
> > > explicitly calling ledtrig_dev_activity().
> > >
> > > Signed-off-by: Maciek Borzecki <maciek.borze...@gmail.com>
> > > ---
> > [..]
> > > +struct ledtrig_dev_data {
> > > + char name[MAX_NAME_LEN];
> > > + dev_t dev;
> > > + struct led_trigger *trig;
> > > + struct list_head node;
> > > +};
> > > +
> > > +/**
> > > + * ledtrig_dev_activity - signal activity on device
> > > + * @dev: device
> > > + *
> > > + * Fires a trigger assigned to @dev device.
> > > + */
> > > +void ledtrig_dev_activity(dev_t dev)
> >
> > It seems a bit strange to me to associate a device LED trigger with
> > dev_t.  Some devices don't expose a dev node, some devices expose
> > multiple dev nodes...
> >
> > Is there a reason why you are not tying to the device model?
> >
> Thanks for the comments.
> 
> The first proof of concept used `sturct device` as parameter in all API
> calls, but then there's a problem of naming of a trigger in a sane
> way. The trigger name followed the same approach as __dev_printk, and
> the naming was done in this fashion:
> 
>sprintf(..., "dev-%s-%s", dev_driver_string(dev), dev_name(dev));
> 
> Then for instance on wandboard, /dev/ttymxc0 and /dev/ttymxc2 would
> appear as `dev-serial-202` and `dev-serial-21ec000`. In my opinion
> this was unnecessarily complicated.

Hmm, maybe we're bikeshedding at this point, but LEDs with those names
seem much more straightfoward to me than a "dev-:" name, for
devices which have done dynamic dev_t allocation.

> Also, if I'm not mistaken, using this approach the partitions on MMC
> card or SATA drive would end up with the same trigger name, as it is a
> single device.

This would only be true if you used _just_ the struct device.  I was
imagining that you'd specify a (struct device, unsigned index) pair.
Better, you could do a (struct device, const char *) pair.

Also, from a lifetime management perspective, it starts to feel like
something that might integrate better as a managed resource (devm_*).

[..]
> Multiple dev nodes will already have different minor numbers, so
> their dev_t is different anyway.

Okay, backing up I don't really see what this API really buys the
consumer.  The dev_t -> struct led_trigger mapping just seems like a
total waste.  Why not just make your ledtrig_dev_add() function return
the struct led_trigger * that the consumer keeps track of?

Maybe seeing an example consumer would provide some clarification.

> As for devices that do not have a dev_t assigned to them one can still
> pass a custom tag in ledtrig_dev_add(). It's just a number so as long as
> there's no collision in numbering things should be fine.

Ensuring no collision will be difficult, especially given that it's most
common that the dynamic allocator is used.  In order to guarantee no
collisions, a user who doesn't expose any device nodes would need to do
their own dev_t allocation...to use this interface.  And that seems
silly to me.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] leds: add device activity LED triggers

[Josh Cartwright]

Hello Maciek-

Some architectural questions below:

On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> The patch adds LED triggers for indicating an activity on a selected
> device. The drivers that intend to use triggers need to register
> respective devices using ledtrig_dev_add(). Triggers are generated by
> explicitly calling ledtrig_dev_activity().
> 
> Signed-off-by: Maciek Borzecki 
> ---
[..]
> +struct ledtrig_dev_data {
> + char name[MAX_NAME_LEN];
> + dev_t dev;
> + struct led_trigger *trig;
> + struct list_head node;
> +};
> +
> +/**
> + * ledtrig_dev_activity - signal activity on device
> + * @dev: device
> + *
> + * Fires a trigger assigned to @dev device.
> + */
> +void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with
dev_t.  Some devices don't expose a dev node, some devices expose
multiple dev nodes...

Is there a reason why you are not tying to the device model?

> +{
> + struct ledtrig_dev_data *dev_trig;
> +
> + if (!down_read_trylock(_list_lock))
> + return;
> +
> + list_for_each_entry(dev_trig, _list, node) {
> + if (dev_trig->dev == dev) {
> + led_trigger_blink_oneshot(dev_trig->trig,
> +   _delay,
> +   _delay,
> +   0);
> + break;
> + }
> + }
> + up_read(_list_lock);
> +}
> +EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2 1/3] leds: add device activity LED triggers

[Josh Cartwright]

Hello Maciek-

Some architectural questions below:

On Thu, Oct 01, 2015 at 04:04:31PM +0200, Maciek Borzecki wrote:
> The patch adds LED triggers for indicating an activity on a selected
> device. The drivers that intend to use triggers need to register
> respective devices using ledtrig_dev_add(). Triggers are generated by
> explicitly calling ledtrig_dev_activity().
> 
> Signed-off-by: Maciek Borzecki 
> ---
[..]
> +struct ledtrig_dev_data {
> + char name[MAX_NAME_LEN];
> + dev_t dev;
> + struct led_trigger *trig;
> + struct list_head node;
> +};
> +
> +/**
> + * ledtrig_dev_activity - signal activity on device
> + * @dev: device
> + *
> + * Fires a trigger assigned to @dev device.
> + */
> +void ledtrig_dev_activity(dev_t dev)

It seems a bit strange to me to associate a device LED trigger with
dev_t.  Some devices don't expose a dev node, some devices expose
multiple dev nodes...

Is there a reason why you are not tying to the device model?

> +{
> + struct ledtrig_dev_data *dev_trig;
> +
> + if (!down_read_trylock(_list_lock))
> + return;
> +
> + list_for_each_entry(dev_trig, _list, node) {
> + if (dev_trig->dev == dev) {
> + led_trigger_blink_oneshot(dev_trig->trig,
> +   _delay,
> +   _delay,
> +   0);
> + break;
> + }
> + }
> + up_read(_list_lock);
> +}
> +EXPORT_SYMBOL(ledtrig_dev_activity);

Not _GPL?

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point

[Josh Cartwright]

On Tue, Sep 29, 2015 at 01:00:45PM -0700, Brian Norris wrote:
> On Tue, Sep 29, 2015 at 12:55:39PM -0700, Brian Norris wrote:
> 
> ...
> 
> Seems Ben Shelton has moved on to other things...

He has, but there are others paying attention who want this feature :).

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point

[Josh Cartwright]

On Tue, Sep 29, 2015 at 01:00:45PM -0700, Brian Norris wrote:
> On Tue, Sep 29, 2015 at 12:55:39PM -0700, Brian Norris wrote:
> 
> ...
> 
> Seems Ben Shelton has moved on to other things...

He has, but there are others paying attention who want this feature :).

  Josh


signature.asc
Description: PGP signature

Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803

[Josh Cartwright]

Hello Alexandre-

Few comments below.

On Sat, Sep 26, 2015 at 03:54:39PM +0200, Alexandre Belloni wrote:
> This driver supports the following functions:
>  - reading and settings time
>  - alarms when connected to an IRQ
>  - reading and clearing the voltage low flags
>  - nvram
> 
> Signed-off-by: Alexandre Belloni 
> ---
[..]
> +static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
> +{
> + struct i2c_client *client = dev_id;
> + struct rv8803_data *rv8803 = i2c_get_clientdata(client);
> + unsigned long events = 0;
> + u8 flags;
> +
> + flags = i2c_smbus_read_byte_data(client, RV8803_FLAG);
> + if (flags <= 0)
> + return IRQ_HANDLED;

Returning IRQ_HANDLED when no interrupt condition is met.  That seems
like a bad idea.

> + if (flags & RV8803_FLAG_V1F)
> + dev_warn(>dev, "Voltage low, temperature compensation 
> stopped.\n");
> +
> + if (flags & RV8803_FLAG_V2F)
> + dev_warn(>dev, "Voltage low, data loss detected.\n");
> +
> + if (flags & RV8803_FLAG_TF) {
> + flags &= ~RV8803_FLAG_TF;
> + rv8803->ctrl &= ~RV8803_CTRL_TIE;
> + events |= RTC_PF;
> + }
> +
> + if (flags & RV8803_FLAG_AF) {
> + flags &= ~RV8803_FLAG_AF;
> + rv8803->ctrl &= ~RV8803_CTRL_AIE;
> + events |= RTC_AF;
> + }
> +
> + if (flags & RV8803_FLAG_UF) {
> + flags &= ~RV8803_FLAG_UF;
> + rv8803->ctrl &= ~RV8803_CTRL_UIE;
> + events |= RTC_UF;
> + }
> +
> + if (events) {
> + rtc_update_irq(rv8803->rtc, 1, events);
> + i2c_smbus_write_byte_data(client, RV8803_FLAG, flags);

How are the many read-modify-write cycles for flags safe without any
form of synchronization?  (Especially given the interrupt handler isn't
under ops_lock).

> + i2c_smbus_write_byte_data(rv8803->client, RV8803_CTRL,
> +   rv8803->ctrl);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
[..]
> +static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct rv8803_data *rv8803 = dev_get_drvdata(dev);
> + struct i2c_client *client = rv8803->client;
> + u8 alarmvals[3];
> + int flags, ret;
> +
> + if (client->irq <= 0)
> + return -EINVAL;

It'd be cleaner just to have a second set of rtc_class_ops that can be
switched between based on whether a valid interrupt is specified.

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

Re: [PATCH] rtc: Add a driver for Micro Crystal RV8803

[Josh Cartwright]

Hello Alexandre-

Few comments below.

On Sat, Sep 26, 2015 at 03:54:39PM +0200, Alexandre Belloni wrote:
> This driver supports the following functions:
>  - reading and settings time
>  - alarms when connected to an IRQ
>  - reading and clearing the voltage low flags
>  - nvram
> 
> Signed-off-by: Alexandre Belloni 
> ---
[..]
> +static irqreturn_t rv8803_handle_irq(int irq, void *dev_id)
> +{
> + struct i2c_client *client = dev_id;
> + struct rv8803_data *rv8803 = i2c_get_clientdata(client);
> + unsigned long events = 0;
> + u8 flags;
> +
> + flags = i2c_smbus_read_byte_data(client, RV8803_FLAG);
> + if (flags <= 0)
> + return IRQ_HANDLED;

Returning IRQ_HANDLED when no interrupt condition is met.  That seems
like a bad idea.

> + if (flags & RV8803_FLAG_V1F)
> + dev_warn(>dev, "Voltage low, temperature compensation 
> stopped.\n");
> +
> + if (flags & RV8803_FLAG_V2F)
> + dev_warn(>dev, "Voltage low, data loss detected.\n");
> +
> + if (flags & RV8803_FLAG_TF) {
> + flags &= ~RV8803_FLAG_TF;
> + rv8803->ctrl &= ~RV8803_CTRL_TIE;
> + events |= RTC_PF;
> + }
> +
> + if (flags & RV8803_FLAG_AF) {
> + flags &= ~RV8803_FLAG_AF;
> + rv8803->ctrl &= ~RV8803_CTRL_AIE;
> + events |= RTC_AF;
> + }
> +
> + if (flags & RV8803_FLAG_UF) {
> + flags &= ~RV8803_FLAG_UF;
> + rv8803->ctrl &= ~RV8803_CTRL_UIE;
> + events |= RTC_UF;
> + }
> +
> + if (events) {
> + rtc_update_irq(rv8803->rtc, 1, events);
> + i2c_smbus_write_byte_data(client, RV8803_FLAG, flags);

How are the many read-modify-write cycles for flags safe without any
form of synchronization?  (Especially given the interrupt handler isn't
under ops_lock).

> + i2c_smbus_write_byte_data(rv8803->client, RV8803_CTRL,
> +   rv8803->ctrl);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
[..]
> +static int rv8803_get_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> + struct rv8803_data *rv8803 = dev_get_drvdata(dev);
> + struct i2c_client *client = rv8803->client;
> + u8 alarmvals[3];
> + int flags, ret;
> +
> + if (client->irq <= 0)
> + return -EINVAL;

It'd be cleaner just to have a second set of rtc_class_ops that can be
switched between based on whether a valid interrupt is specified.

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

Re: [PATCH v11 3/4] add FPGA manager core

[Josh Cartwright]

On Wed, Sep 23, 2015 at 12:10:13PM -0500, atull wrote:
> On Tue, 22 Sep 2015, Josh Cartwright wrote:
[..]
> > > +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> > > +{
> > > + struct fpga_manager *mgr;
> > > + struct device *dev;
> > > +
> > > + if (!node)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + dev = class_find_device(fpga_mgr_class, NULL, node,
> > > + fpga_mgr_of_node_match);
> > > + if (!dev)
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + mgr = to_fpga_manager(dev);
> > > + put_device(dev);
> > 
> > Who's ensuring the FPGA manager device's lifetime between _get and _put?
> 
> Well... get_device and put_device are not sufficient?

Sorry if I was too opaque.

You've just put_device()'d the only reference to the device you had
here, so nothing is preventing from the device from being ripped away
while it's being used.

You should remove the put_device() call here, and add a corresponding
put_device() in fpga_mgr_put().

The module refcounting is also a bit strange, as you are
acquiring/releasing a reference to THIS_MODULE, but you also should care
about the lower-level driver's module refcount.

[..]
> > > + dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> > > + if (dt_label)
> > > + ret = dev_set_name(>dev, "%s", dt_label);
> > > + else
> > > + ret = dev_set_name(>dev, "fpga%d", id);
> > 
> > I'm wondering if an alias {} node is better for this.
> 
> I could look into that.  Is there an example of that you particularly
> like for this?

Look at i2c's usage of the of_alias_*() functions.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] add FPGA manager core

[Josh Cartwright]

On Wed, Sep 23, 2015 at 12:10:13PM -0500, atull wrote:
> On Tue, 22 Sep 2015, Josh Cartwright wrote:
[..]
> > > +struct fpga_manager *of_fpga_mgr_get(struct device_node *node)
> > > +{
> > > + struct fpga_manager *mgr;
> > > + struct device *dev;
> > > +
> > > + if (!node)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + dev = class_find_device(fpga_mgr_class, NULL, node,
> > > + fpga_mgr_of_node_match);
> > > + if (!dev)
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + mgr = to_fpga_manager(dev);
> > > + put_device(dev);
> > 
> > Who's ensuring the FPGA manager device's lifetime between _get and _put?
> 
> Well... get_device and put_device are not sufficient?

Sorry if I was too opaque.

You've just put_device()'d the only reference to the device you had
here, so nothing is preventing from the device from being ripped away
while it's being used.

You should remove the put_device() call here, and add a corresponding
put_device() in fpga_mgr_put().

The module refcounting is also a bit strange, as you are
acquiring/releasing a reference to THIS_MODULE, but you also should care
about the lower-level driver's module refcount.

[..]
> > > + dt_label = of_get_property(mgr->dev.of_node, "label", NULL);
> > > + if (dt_label)
> > > + ret = dev_set_name(>dev, "%s", dt_label);
> > > + else
> > > + ret = dev_set_name(>dev, "fpga%d", id);
> > 
> > I'm wondering if an alias {} node is better for this.
> 
> I could look into that.  Is there an example of that you particularly
> like for this?

Look at i2c's usage of the of_alias_*() functions.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v11 4/4] fpga manager: add driver for socfpga fpga manager

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:11AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> Add driver to fpga manager framework to allow configuration
> of FPGA in Altera SoCFPGA parts.
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
> Acked-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/Kconfig
> @@ -11,4 +11,14 @@ config FPGA
> kernel.  The FPGA framework adds a FPGA manager class and FPGA
> manager drivers.
>  
> +if FPGA

FPGA is unconditionally set here, otherwise drivers/fpga/Kconfig
wouldn't even be considered.

> +
> +config FPGA_MGR_SOCFPGA
> + tristate "Altera SOCFPGA FPGA Manager"
> + depends on ARCH_SOCFPGA
> + help
> +   FPGA manager driver support for Altera SOCFPGA.
> +
> +endif # FPGA
> +
>  endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 3313c52..ba6c5c5 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,3 +6,4 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> new file mode 100644
> index 000..706b80d
> --- /dev/null
> +++ b/drivers/fpga/socfpga.c
[..]
> +/*
> + * Step 9: write data to the FPGA data register
> + */
> +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct socfpga_fpga_priv *priv = mgr->priv;
> + u32 *buffer_32 = (u32 *)buf;

Seems sketchy from an endianess perspective, but it may be okay if
SOCFPGA doesn't support BE (which my follow up question would be: why
not?).  Same thing applies with seemingly cavalier usages of the
__raw_readl/writel variants.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] add FPGA manager core

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:10AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> API to support programming FPGA's.
> 
> The following functions are exported as GPL:
> * fpga_mgr_buf_load
>Load fpga from image in buffer
> 
> * fpga_mgr_firmware_load
>Request firmware and load it to the FPGA.
> 
> * fpga_mgr_register
> * fpga_mgr_unregister
>FPGA device drivers can be added by calling
>fpga_mgr_register() to register a set of
>fpga_manager_ops to do device specific stuff.
> 
> * of_fpga_mgr_get
> * fpga_mgr_put
>Get/put a reference to a fpga manager.
> 
> The following sysfs files are created:
> * /sys/class/fpga_manager//name
>   Name of low level driver.

Don't 'struct device's already have a name?  Why do you need another name
attribute?

> 
> * /sys/class/fpga_manager//state
>   State of fpga manager
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
[..]
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,382 @@
[..]
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> +   size_t count)
> +{
> + struct device *dev = >dev;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;

This seems overly defensive.

[..]
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> +const char *image_name)
> +{
> + struct device *dev = >dev;
> + const struct firmware *fw;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;

Again; I'm of the opinion this is needlessly defensive.

> +
> + dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
> +
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> + ret = request_firmware(, image_name, dev);
> + if (ret) {
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> + dev_err(dev, "Error requesting firmware %s\n", image_name);
> + return ret;
> + }
> +
> + ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
> + if (ret)
> + return ret;
> +
> + release_firmware(fw);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +static const char * const state_str[] = {
> + [FPGA_MGR_STATE_UNKNOWN] =  "unknown",
> + [FPGA_MGR_STATE_POWER_OFF] ="power off",
> + [FPGA_MGR_STATE_POWER_UP] = "power up",
> + [FPGA_MGR_STATE_RESET] ="reset",
> +
> + /* requesting FPGA image from firmware */
> + [FPGA_MGR_STATE_FIRMWARE_REQ] = "firmware request",
> + [FPGA_MGR_STATE_FIRMWARE_REQ_ERR] = "firmware request error",
> +
> + /* Preparing FPGA to receive image */
> + [FPGA_MGR_STATE_WRITE_INIT] =   "write init",
> + [FPGA_MGR_STATE_WRITE_INIT_ERR] =   "write init error",
> +
> + /* Writing image to FPGA */
> + [FPGA_MGR_STATE_WRITE] ="write",
> + [FPGA_MGR_STATE_WRITE_ERR] ="write error",
> +
> + /* Finishing configuration after image has been written */
> + [FPGA_MGR_STATE_WRITE_COMPLETE] =   "write complete",
> + [FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =   "write complete error",
> +
> + /* FPGA reports to be in normal operating mode */
> + [FPGA_MGR_STATE_OPERATING] ="operating",
> +};

Is it really necessary to expose the whole FPGA manager state machine to
userspace?  Is the only justification "for debugging"?

If so, that seems pretty short-sighted, as it then makes the state
machine part of the stable usermode API.  It even makes less sense given
this patchsets current state: configuration of the FPGA is not something
that userspace is directly triggering.

> +
> +static ssize_t name_show(struct device *dev,
> +  struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", mgr->name);
> +}
> +
> +static ssize_t state_show(struct device *dev,
> +   struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", state_str[mgr->state]);
> +}

Is it possible that the state of the FPGA has changed since the last
time we've done an update?  Should the lower-level drivers' state()
callback be invoked here?

> +
> +static DEVICE_ATTR_RO(name);
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> + _attr_name.attr,
> + _attr_state.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static int fpga_mgr_of_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
> + * @node:device node
> + *
> + * Given a device node, get an exclusive reference to a fpga mgr.
> + *
> + * Return: fpga manager struct or IS_ERR() condition containing error code.
> + */
> +struct 

Re: [PATCH v11 3/4] add FPGA manager core

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:10AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> API to support programming FPGA's.
> 
> The following functions are exported as GPL:
> * fpga_mgr_buf_load
>Load fpga from image in buffer
> 
> * fpga_mgr_firmware_load
>Request firmware and load it to the FPGA.
> 
> * fpga_mgr_register
> * fpga_mgr_unregister
>FPGA device drivers can be added by calling
>fpga_mgr_register() to register a set of
>fpga_manager_ops to do device specific stuff.
> 
> * of_fpga_mgr_get
> * fpga_mgr_put
>Get/put a reference to a fpga manager.
> 
> The following sysfs files are created:
> * /sys/class/fpga_manager//name
>   Name of low level driver.

Don't 'struct device's already have a name?  Why do you need another name
attribute?

> 
> * /sys/class/fpga_manager//state
>   State of fpga manager
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
[..]
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,382 @@
[..]
> +int fpga_mgr_buf_load(struct fpga_manager *mgr, u32 flags, const char *buf,
> +   size_t count)
> +{
> + struct device *dev = >dev;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;

This seems overly defensive.

[..]
> +int fpga_mgr_firmware_load(struct fpga_manager *mgr, u32 flags,
> +const char *image_name)
> +{
> + struct device *dev = >dev;
> + const struct firmware *fw;
> + int ret;
> +
> + if (!mgr)
> + return -ENODEV;

Again; I'm of the opinion this is needlessly defensive.

> +
> + dev_info(dev, "writing %s to %s\n", image_name, mgr->name);
> +
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
> +
> + ret = request_firmware(, image_name, dev);
> + if (ret) {
> + mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
> + dev_err(dev, "Error requesting firmware %s\n", image_name);
> + return ret;
> + }
> +
> + ret = fpga_mgr_buf_load(mgr, flags, fw->data, fw->size);
> + if (ret)
> + return ret;
> +
> + release_firmware(fw);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_load);
> +
> +static const char * const state_str[] = {
> + [FPGA_MGR_STATE_UNKNOWN] =  "unknown",
> + [FPGA_MGR_STATE_POWER_OFF] ="power off",
> + [FPGA_MGR_STATE_POWER_UP] = "power up",
> + [FPGA_MGR_STATE_RESET] ="reset",
> +
> + /* requesting FPGA image from firmware */
> + [FPGA_MGR_STATE_FIRMWARE_REQ] = "firmware request",
> + [FPGA_MGR_STATE_FIRMWARE_REQ_ERR] = "firmware request error",
> +
> + /* Preparing FPGA to receive image */
> + [FPGA_MGR_STATE_WRITE_INIT] =   "write init",
> + [FPGA_MGR_STATE_WRITE_INIT_ERR] =   "write init error",
> +
> + /* Writing image to FPGA */
> + [FPGA_MGR_STATE_WRITE] ="write",
> + [FPGA_MGR_STATE_WRITE_ERR] ="write error",
> +
> + /* Finishing configuration after image has been written */
> + [FPGA_MGR_STATE_WRITE_COMPLETE] =   "write complete",
> + [FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =   "write complete error",
> +
> + /* FPGA reports to be in normal operating mode */
> + [FPGA_MGR_STATE_OPERATING] ="operating",
> +};

Is it really necessary to expose the whole FPGA manager state machine to
userspace?  Is the only justification "for debugging"?

If so, that seems pretty short-sighted, as it then makes the state
machine part of the stable usermode API.  It even makes less sense given
this patchsets current state: configuration of the FPGA is not something
that userspace is directly triggering.

> +
> +static ssize_t name_show(struct device *dev,
> +  struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", mgr->name);
> +}
> +
> +static ssize_t state_show(struct device *dev,
> +   struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", state_str[mgr->state]);
> +}

Is it possible that the state of the FPGA has changed since the last
time we've done an update?  Should the lower-level drivers' state()
callback be invoked here?

> +
> +static DEVICE_ATTR_RO(name);
> +static DEVICE_ATTR_RO(state);
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> + _attr_name.attr,
> + _attr_state.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(fpga_mgr);
> +
> +static int fpga_mgr_of_node_match(struct device *dev, const void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_fpga_mgr_get - get an exclusive reference to a fpga mgr
> + * @node:device node
> + *
> + * Given a device node, get an exclusive reference to a fpga mgr.
> + *
> + * Return: fpga 

Re: [PATCH v11 4/4] fpga manager: add driver for socfpga fpga manager

[Josh Cartwright]

On Tue, Sep 22, 2015 at 10:21:11AM -0500, at...@opensource.altera.com wrote:
> From: Alan Tull 
> 
> Add driver to fpga manager framework to allow configuration
> of FPGA in Altera SoCFPGA parts.
> 
> Signed-off-by: Alan Tull 
> Acked-by: Michal Simek 
> Acked-by: Moritz Fischer 
[..]
> +++ b/drivers/fpga/Kconfig
> @@ -11,4 +11,14 @@ config FPGA
> kernel.  The FPGA framework adds a FPGA manager class and FPGA
> manager drivers.
>  
> +if FPGA

FPGA is unconditionally set here, otherwise drivers/fpga/Kconfig
wouldn't even be considered.

> +
> +config FPGA_MGR_SOCFPGA
> + tristate "Altera SOCFPGA FPGA Manager"
> + depends on ARCH_SOCFPGA
> + help
> +   FPGA manager driver support for Altera SOCFPGA.
> +
> +endif # FPGA
> +
>  endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 3313c52..ba6c5c5 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,3 +6,4 @@
>  obj-$(CONFIG_FPGA)   += fpga-mgr.o
>  
>  # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_SOCFPGA)   += socfpga.o
> diff --git a/drivers/fpga/socfpga.c b/drivers/fpga/socfpga.c
> new file mode 100644
> index 000..706b80d
> --- /dev/null
> +++ b/drivers/fpga/socfpga.c
[..]
> +/*
> + * Step 9: write data to the FPGA data register
> + */
> +static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct socfpga_fpga_priv *priv = mgr->priv;
> + u32 *buffer_32 = (u32 *)buf;

Seems sketchy from an endianess perspective, but it may be okay if
SOCFPGA doesn't support BE (which my follow up question would be: why
not?).  Same thing applies with seemingly cavalier usages of the
__raw_readl/writel variants.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH 1/2] ubifs: Remove dead xattr code

[Josh Cartwright]

On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote:
> On 08/20/2015 04:35 AM, Richard Weinberger wrote:
> >This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
> >("UBIFS: Add security.* XATTR support for the UBIFS").
> 
> Hi Richard,
>   What about a full reverting of this commit. In ubifs, we
> *can* support any namespace of xattr including user, trusted, security
> or other anyone prefixed by any words. But we have a check_namespace()
> in xattr.c to limit what we want to support. That said, if we want to
> "Add security.* XATTR support for the UBIFS", what we need to do is
> just extending the check_namespace() to allow security namespace pass.
> And yes, check_namespace() have been supporting security namespace.

Is this good enough?  Yes, it'd mean that the xattrs end up on disk, but
then who's responsible for invoking the selected LSMs inode_init_security() 
hooks?
AFAICT, we'd still need to invoke security_inode_init_security for newly
created inodes (which, Richard's proposed commit still does).

Thanks,

  Josh (who, admittedly, is neither a filesystem nor security module guy :)


signature.asc
Description: PGP signature

Re: [PATCH 1/2] ubifs: Remove dead xattr code

[Josh Cartwright]

On Thu, Aug 20, 2015 at 10:48:38AM +0800, Dongsheng Yang wrote:
 On 08/20/2015 04:35 AM, Richard Weinberger wrote:
 This is a partial revert of commit d7f0b70d30ffb9bbe6b8a3e1035cf0b79965ef53
 (UBIFS: Add security.* XATTR support for the UBIFS).
 
 Hi Richard,
   What about a full reverting of this commit. In ubifs, we
 *can* support any namespace of xattr including user, trusted, security
 or other anyone prefixed by any words. But we have a check_namespace()
 in xattr.c to limit what we want to support. That said, if we want to
 Add security.* XATTR support for the UBIFS, what we need to do is
 just extending the check_namespace() to allow security namespace pass.
 And yes, check_namespace() have been supporting security namespace.

Is this good enough?  Yes, it'd mean that the xattrs end up on disk, but
then who's responsible for invoking the selected LSMs inode_init_security() 
hooks?
AFAICT, we'd still need to invoke security_inode_init_security for newly
created inodes (which, Richard's proposed commit still does).

Thanks,

  Josh (who, admittedly, is neither a filesystem nor security module guy :)


signature.asc
Description: PGP signature

Re: [PATCH v2] nohz: prevent tilegx network driver interrupts

[Josh Cartwright]

On Fri, Jul 10, 2015 at 07:06:23PM -0400, Chris Metcalf wrote:
> On 7/10/2015 6:45 PM, Josh Cartwright wrote:
> >>+static inline const struct cpumask *housekeeping_cpumask(void)
> >>>+{
> >>>+#ifdef CONFIG_NO_HZ_FULL
> >>>+  if (tick_nohz_full_enabled())
> >>>+  return housekeeping_mask;
> >>>+#endif
> >Just a small comment:
> >
> >We can take these checks out from under a #ifdef CONFIG_NO_HZ_FULL
> >check, given that are stubbed tick_nohz_full_enabled() defined above.
> 
> True for the "if" clause, but the "housekeeping_mask" variable is only defined
> under CONFIG_NO_HZ_FULL.

Indeed!  I should have read more carefully.

Sorry for the noise.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] nohz: prevent tilegx network driver interrupts

[Josh Cartwright]

On Fri, Jul 10, 2015 at 03:37:25PM -0400, Chris Metcalf wrote:
> Normally the tilegx networking shim sends irqs to all the cores
> to distribute the load of processing incoming-packet interrupts,
> so that you can get to multiple Gb's of traffic inbound.
> 
> However, in nohz_full mode we don't want to interrupt the
> nohz_full cores by default, so we limit the set of cores we use
> to only the online housekeeping cores.
> 
> To make client code easier to read, we introduce a new nohz_full
> accessor, housekeeping_cpumask(), which returns a pointer to the
> housekeeping_mask if nohz_full is enabled, and otherwise returns
> the cpu_possible_mask.
> 
> Signed-off-by: Chris Metcalf 
> ---
[..]
> +static inline const struct cpumask *housekeeping_cpumask(void)
> +{
> +#ifdef CONFIG_NO_HZ_FULL
> + if (tick_nohz_full_enabled())
> + return housekeeping_mask;
> +#endif

Just a small comment:

We can take these checks out from under a #ifdef CONFIG_NO_HZ_FULL
check, given that are stubbed tick_nohz_full_enabled() defined above.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] nohz: prevent tilegx network driver interrupts

[Josh Cartwright]

On Fri, Jul 10, 2015 at 03:37:25PM -0400, Chris Metcalf wrote:
 Normally the tilegx networking shim sends irqs to all the cores
 to distribute the load of processing incoming-packet interrupts,
 so that you can get to multiple Gb's of traffic inbound.
 
 However, in nohz_full mode we don't want to interrupt the
 nohz_full cores by default, so we limit the set of cores we use
 to only the online housekeeping cores.
 
 To make client code easier to read, we introduce a new nohz_full
 accessor, housekeeping_cpumask(), which returns a pointer to the
 housekeeping_mask if nohz_full is enabled, and otherwise returns
 the cpu_possible_mask.
 
 Signed-off-by: Chris Metcalf cmetc...@ezchip.com
 ---
[..]
 +static inline const struct cpumask *housekeeping_cpumask(void)
 +{
 +#ifdef CONFIG_NO_HZ_FULL
 + if (tick_nohz_full_enabled())
 + return housekeeping_mask;
 +#endif

Just a small comment:

We can take these checks out from under a #ifdef CONFIG_NO_HZ_FULL
check, given that are stubbed tick_nohz_full_enabled() defined above.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2] nohz: prevent tilegx network driver interrupts

[Josh Cartwright]

On Fri, Jul 10, 2015 at 07:06:23PM -0400, Chris Metcalf wrote:
 On 7/10/2015 6:45 PM, Josh Cartwright wrote:
 +static inline const struct cpumask *housekeeping_cpumask(void)
 +{
 +#ifdef CONFIG_NO_HZ_FULL
 +  if (tick_nohz_full_enabled())
 +  return housekeeping_mask;
 +#endif
 Just a small comment:
 
 We can take these checks out from under a #ifdef CONFIG_NO_HZ_FULL
 check, given that are stubbed tick_nohz_full_enabled() defined above.
 
 True for the if clause, but the housekeeping_mask variable is only defined
 under CONFIG_NO_HZ_FULL.

Indeed!  I should have read more carefully.

Sorry for the noise.

  Josh


signature.asc
Description: PGP signature

Re: [PATCH v7 0/3] Add arm pl353 smc nand driver for xilinx zynq soc

[Josh Cartwright]

On Mon, Jun 08, 2015 at 11:38:35PM +0530, Punnaiah Choudary Kalluri wrote:
> The following patches add arm pl353 static memory controller driver for
> xilinx zynq soc. The arm pl353 smc supports two interfaces i.e nand and
> nor/sram memory interfaces. The current implementation supports only a
> single SMC instance and nand specific configuration.

What's the integration plan for this guy?  Looks like we missed 4.2.
It'd be swell to get have NAND support land for Zynq in 4.3.

Assuming there is nothing else holding it back:

Brian- I'm assuming you'll want to take this through your tree.

It looks like most of the stuff in drivers/memory have historically gone
through arm-soc/drivers, Michal- does it make sense for you to pick up
http://lkml.kernel.org/r/1433786857-32575-1-git-send-email-punn...@xilinx.com?

Thanks,
  Josh


signature.asc
Description: PGP signature

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Josh Cartwright]

On Wed, Jul 08, 2015 at 01:54:34PM +0200, Richard Cochran wrote:
> On Mon, Jul 06, 2015 at 03:44:58PM -0500, Josh Cartwright wrote:
> > It's difficult to make too many judgements without seeing how a driver
> > might implement this; is there another patchset that shows how a driver
> > implements this?
> 
> The interface is certainly clear enough to me.  The details of
> obtaining a cross time stamp from the hardware will remain hidden
> behind the interface in any case.

It's unusual to see interfaces added like this without also seeing users
at the same time to see how the pieces fit together.  Should I have
assumed this was a PATCH RFC for just the API?

I'm afraid this is the tip of a much larger iceberg.  If a device is
doing hardware crosstimestamping, what is the hardwares view of this
"system clock"?  How is it tied into the kernels' timekeeping?  How is
it ensured that same clock the hardware sees is the kernel's actively
selected clocksource?

You get this for free in software with getnstimeofday(), by pushing it
to hardware, the boundaries of responsibilities are all
unclear...without seeing the whole picture.

> > > - int rsv[14];   /* Reserved for future use. */
> > > + /* Whether the clock supports precise system-device cross timestamps */
> > > + int precise_timestamping;
> >
> > Perhaps now is a good time to add an unsigned int 'flags' member instead,
> > and start allocating bits.
>
> Considering the rate of growth for the PHC stuff, I am not worried
> about spending a whole 'int' on this.

Fair enough!

Thanks,

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

Re: [PATCH v2 1/1] Added additional callback to ptp_clock_info:

[Josh Cartwright]

On Wed, Jul 08, 2015 at 01:54:34PM +0200, Richard Cochran wrote:
 On Mon, Jul 06, 2015 at 03:44:58PM -0500, Josh Cartwright wrote:
  It's difficult to make too many judgements without seeing how a driver
  might implement this; is there another patchset that shows how a driver
  implements this?
 
 The interface is certainly clear enough to me.  The details of
 obtaining a cross time stamp from the hardware will remain hidden
 behind the interface in any case.

It's unusual to see interfaces added like this without also seeing users
at the same time to see how the pieces fit together.  Should I have
assumed this was a PATCH RFC for just the API?

I'm afraid this is the tip of a much larger iceberg.  If a device is
doing hardware crosstimestamping, what is the hardwares view of this
system clock?  How is it tied into the kernels' timekeeping?  How is
it ensured that same clock the hardware sees is the kernel's actively
selected clocksource?

You get this for free in software with getnstimeofday(), by pushing it
to hardware, the boundaries of responsibilities are all
unclear...without seeing the whole picture.

   - int rsv[14];   /* Reserved for future use. */
   + /* Whether the clock supports precise system-device cross timestamps */
   + int precise_timestamping;
 
  Perhaps now is a good time to add an unsigned int 'flags' member instead,
  and start allocating bits.

 Considering the rate of growth for the PHC stuff, I am not worried
 about spending a whole 'int' on this.

Fair enough!

Thanks,

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

Re: [PATCH v7 0/3] Add arm pl353 smc nand driver for xilinx zynq soc

[Josh Cartwright]

On Mon, Jun 08, 2015 at 11:38:35PM +0530, Punnaiah Choudary Kalluri wrote:
 The following patches add arm pl353 static memory controller driver for
 xilinx zynq soc. The arm pl353 smc supports two interfaces i.e nand and
 nor/sram memory interfaces. The current implementation supports only a
 single SMC instance and nand specific configuration.

What's the integration plan for this guy?  Looks like we missed 4.2.
It'd be swell to get have NAND support land for Zynq in 4.3.

Assuming there is nothing else holding it back:

Brian- I'm assuming you'll want to take this through your tree.

It looks like most of the stuff in drivers/memory have historically gone
through arm-soc/drivers, Michal- does it make sense for you to pick up
http://lkml.kernel.org/r/1433786857-32575-1-git-send-email-punn...@xilinx.com?

Thanks,
  Josh


signature.asc
Description: PGP signature