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

2015-12-08 Thread Nicolas Ferre 
Le 08/12/2015 10:21, Neil Armstrong a écrit :
> Hi Josh,
> 
> 2015-12-07 20:32 GMT+01:00 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
> 
> I see the point, but according to the User Guide :
>> User I/O Register
>> The MACB design provides up to 16 inputs and 16 outputs,
>> for which the state of the I/O may
>> be read or set under the control of the processor interface.
>> If the user I/O is disabled as a configuration option, this address space is 
>> defined
>> as reserved, and hence will be a read-only register of value 0x0.
> 
> On the design I worked on, the macb_user_* signals were commented,
> thus disabling this register.
> 
> The implementation is not mandatory, and the "generic" macb compatible
> "cdns,macb" should disable
> usage of USRIO register by default and be only used for platform
> specific macb instances...
> 
> Is it OK if I add a new 'caps' bit and use it for the "generic" macb instance 
> ?

I would say no as some platform may already use this compatibility
string. So If you need a different capability set, please create a new
compatible string and use this one.

> For the device tree property, it should be safe to have the generic
> instances of macb and gem to
> rely on these properties instead of hardcoded instances.
> (it's the biggest aim of device tree, no ? no more hardcoded 'caps' bit ?)
> The "no-usrio" and other should eventually map 'caps' bits along the
> generic instances.

It has been decided a log time ago to use these capabilities and to have
mixed approach to the actual configuration of the IP:
- from the compatibility string
- from the configuration registers.

I may be sometimes challenging to figure out where the final property
comes from but it has proven to be pretty well adapted to any kind of
situation.

So let's continue with this and not insert additional properties to this
binding.

Best regards,
-- 
Nicolas Ferre
--
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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-08 Thread Neil Armstrong 
Hi Josh,

2015-12-07 20:32 GMT+01:00 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

I see the point, but according to the User Guide :
>User I/O Register
> The MACB design provides up to 16 inputs and 16 outputs,
> for which the state of the I/O may
> be read or set under the control of the processor interface.
> If the user I/O is disabled as a configuration option, this address space is 
> defined
> as reserved, and hence will be a read-only register of value 0x0.

On the design I worked on, the macb_user_* signals were commented,
thus disabling this register.

The implementation is not mandatory, and the "generic" macb compatible
"cdns,macb" should disable
usage of USRIO register by default and be only used for platform
specific macb instances...

Is it OK if I add a new 'caps' bit and use it for the "generic" macb instance ?

For the device tree property, it should be safe to have the generic
instances of macb and gem to
rely on these properties instead of hardcoded instances.
(it's the biggest aim of device tree, no ? no more hardcoded 'caps' bit ?)
The "no-usrio" and other should eventually map 'caps' bits along the
generic instances.

Neil
--
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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-08 Thread Neil Armstrong 
Hi Josh,

2015-12-07 20:32 GMT+01:00 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

I see the point, but according to the User Guide :
>User I/O Register
> The MACB design provides up to 16 inputs and 16 outputs,
> for which the state of the I/O may
> be read or set under the control of the processor interface.
> If the user I/O is disabled as a configuration option, this address space is 
> defined
> as reserved, and hence will be a read-only register of value 0x0.

On the design I worked on, the macb_user_* signals were commented,
thus disabling this register.

The implementation is not mandatory, and the "generic" macb compatible
"cdns,macb" should disable
usage of USRIO register by default and be only used for platform
specific macb instances...

Is it OK if I add a new 'caps' bit and use it for the "generic" macb instance ?

For the device tree property, it should be safe to have the generic
instances of macb and gem to
rely on these properties instead of hardcoded instances.
(it's the biggest aim of device tree, no ? no more hardcoded 'caps' bit ?)
The "no-usrio" and other should eventually map 'caps' bits along the
generic instances.

Neil
--
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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-08 Thread Nicolas Ferre 
Le 08/12/2015 10:21, Neil Armstrong a écrit :
> Hi Josh,
> 
> 2015-12-07 20:32 GMT+01:00 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
> 
> I see the point, but according to the User Guide :
>> User I/O Register
>> The MACB design provides up to 16 inputs and 16 outputs,
>> for which the state of the I/O may
>> be read or set under the control of the processor interface.
>> If the user I/O is disabled as a configuration option, this address space is 
>> defined
>> as reserved, and hence will be a read-only register of value 0x0.
> 
> On the design I worked on, the macb_user_* signals were commented,
> thus disabling this register.
> 
> The implementation is not mandatory, and the "generic" macb compatible
> "cdns,macb" should disable
> usage of USRIO register by default and be only used for platform
> specific macb instances...
> 
> Is it OK if I add a new 'caps' bit and use it for the "generic" macb instance 
> ?

I would say no as some platform may already use this compatibility
string. So If you need a different capability set, please create a new
compatible string and use this one.

> For the device tree property, it should be safe to have the generic
> instances of macb and gem to
> rely on these properties instead of hardcoded instances.
> (it's the biggest aim of device tree, no ? no more hardcoded 'caps' bit ?)
> The "no-usrio" and other should eventually map 'caps' bits along the
> generic instances.

It has been decided a log time ago to use these capabilities and to have
mixed approach to the actual configuration of the IP:
- from the compatibility string
- from the configuration registers.

I may be sometimes challenging to figure out where the final property
comes from but it has proven to be pretty well adapted to any kind of
situation.

So let's continue with this and not insert additional properties to this
binding.

Best regards,
-- 
Nicolas Ferre
--
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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-07 Thread 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

2015-12-07 Thread David Miller 
From: Neil Armstrong 
Date: Mon,  7 Dec 2015 11:58:33 +0100

> - regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + if (!of_property_read_bool(bp->pdev->dev.of_node, "no-usrio")) {
> + regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + }

Single statement basic blocks shall not be enclosed in curly braces.
--
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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-07 Thread Neil Armstrong 
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.
---
 drivers/net/ethernet/cadence/macb.c | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 169059c..3897620 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2122,7 +2122,9 @@ static void macb_get_regs(struct net_device *dev, struct 
ethtool_regs *regs,
regs_buff[10] = macb_tx_dma(>queues[0], tail);
regs_buff[11] = macb_tx_dma(>queues[0], head);
 
-   regs_buff[12] = macb_or_gem_readl(bp, USRIO);
+   if (!of_property_read_bool(bp->pdev->dev.of_node, "no-usrio")) {
+   regs_buff[12] = macb_or_gem_readl(bp, USRIO);
+   }
if (macb_is_gem(bp)) {
regs_buff[13] = gem_readl(bp, DMACFG);
}
@@ -2401,19 +2403,22 @@ static int macb_init(struct platform_device *pdev)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;
 
-   val = 0;
-   if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
-   val = GEM_BIT(RGMII);
-   else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
-(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
-   val = MACB_BIT(RMII);
-   else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
-   val = MACB_BIT(MII);
+   /* Some platform do not implement the USRIO register */
+   if (!of_property_read_bool(pdev->dev.of_node, "no-usrio")) {
+   val = 0;
+   if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
+   val = GEM_BIT(RGMII);
+   else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
+(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+   val = MACB_BIT(RMII);
+   else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+   val = MACB_BIT(MII);
 
-   if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
-   val |= MACB_BIT(CLKEN);
+   if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
+   val |= MACB_BIT(CLKEN);
 
-   macb_or_gem_writel(bp, USRIO, val);
+   macb_or_gem_writel(bp, USRIO, val);
+   }
 
/* Set MII management clock divider */
val = macb_mdc_clk_div(bp);
-- 
1.9.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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-07 Thread Neil Armstrong 
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.
---
 drivers/net/ethernet/cadence/macb.c | 29 +
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c 
b/drivers/net/ethernet/cadence/macb.c
index 169059c..3897620 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2122,7 +2122,9 @@ static void macb_get_regs(struct net_device *dev, struct 
ethtool_regs *regs,
regs_buff[10] = macb_tx_dma(>queues[0], tail);
regs_buff[11] = macb_tx_dma(>queues[0], head);
 
-   regs_buff[12] = macb_or_gem_readl(bp, USRIO);
+   if (!of_property_read_bool(bp->pdev->dev.of_node, "no-usrio")) {
+   regs_buff[12] = macb_or_gem_readl(bp, USRIO);
+   }
if (macb_is_gem(bp)) {
regs_buff[13] = gem_readl(bp, DMACFG);
}
@@ -2401,19 +2403,22 @@ static int macb_init(struct platform_device *pdev)
dev->hw_features &= ~NETIF_F_SG;
dev->features = dev->hw_features;
 
-   val = 0;
-   if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
-   val = GEM_BIT(RGMII);
-   else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
-(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
-   val = MACB_BIT(RMII);
-   else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
-   val = MACB_BIT(MII);
+   /* Some platform do not implement the USRIO register */
+   if (!of_property_read_bool(pdev->dev.of_node, "no-usrio")) {
+   val = 0;
+   if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
+   val = GEM_BIT(RGMII);
+   else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII &&
+(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+   val = MACB_BIT(RMII);
+   else if (!(bp->caps & MACB_CAPS_USRIO_DEFAULT_IS_MII))
+   val = MACB_BIT(MII);
 
-   if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
-   val |= MACB_BIT(CLKEN);
+   if (bp->caps & MACB_CAPS_USRIO_HAS_CLKEN)
+   val |= MACB_BIT(CLKEN);
 
-   macb_or_gem_writel(bp, USRIO, val);
+   macb_or_gem_writel(bp, USRIO, val);
+   }
 
/* Set MII management clock divider */
val = macb_mdc_clk_div(bp);
-- 
1.9.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 net 1/2] net: cadence: macb: Disable USRIO register on some platforms

2015-12-07 Thread 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

2015-12-07 Thread David Miller 
From: Neil Armstrong 
Date: Mon,  7 Dec 2015 11:58:33 +0100

> - regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + if (!of_property_read_bool(bp->pdev->dev.of_node, "no-usrio")) {
> + regs_buff[12] = macb_or_gem_readl(bp, USRIO);
> + }

Single statement basic blocks shall not be enclosed in curly braces.
--
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/