[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Northstar devices have MDIO bus that may contain various PHYs attached.
A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index acee36a61004..6a2afe7880ae 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -320,6 +320,13 @@
 		};
 	};
 
+	mdio@18003000 {
+		compatible = "brcm,iproc-mdio";
+		reg = <0x18003000 0x8>;
+		#size-cells = <1>;
+		#address-cells = <0>;
+	};
+
 	i2c0: i2c@18009000 {
 		compatible = "brcm,iproc-i2c";
 		reg = <0x18009000 0x50>;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

From: Rafa? Mi?ecki <rafal@milecki.pl>

Northstar devices have MDIO bus that may contain various PHYs attached.
A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).

Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
---
 arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index acee36a61004..6a2afe7880ae 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -320,6 +320,13 @@
 		};
 	};
 
+	mdio at 18003000 {
+		compatible = "brcm,iproc-mdio";
+		reg = <0x18003000 0x8>;
+		#size-cells = <1>;
+		#address-cells = <0>;
+	};
+
 	i2c0: i2c at 18009000 {
 		compatible = "brcm,iproc-i2c";
 		reg = <0x18009000 0x50>;
-- 
2.11.0

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> Northstar devices have MDIO bus that may contain various PHYs attached.
> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
> 
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
> index acee36a61004..6a2afe7880ae 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -320,6 +320,13 @@
>  		};
>  	};
>  
> +	mdio@18003000 {
> +		compatible = "brcm,iproc-mdio";
> +		reg = <0x18003000 0x8>;
> +		#size-cells = <1>;
> +		#address-cells = <0>;
> +	};

This looks fine, but usually the block should be enabled on a per-board
basis, such that there should be a status = "disabled" property here by
default.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
> From: Rafa? Mi?ecki <rafal@milecki.pl>
> 
> Northstar devices have MDIO bus that may contain various PHYs attached.
> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
> 
> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
> ---
>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
> index acee36a61004..6a2afe7880ae 100644
> --- a/arch/arm/boot/dts/bcm5301x.dtsi
> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
> @@ -320,6 +320,13 @@
>  		};
>  	};
>  
> +	mdio at 18003000 {
> +		compatible = "brcm,iproc-mdio";
> +		reg = <0x18003000 0x8>;
> +		#size-cells = <1>;
> +		#address-cells = <0>;
> +	};

This looks fine, but usually the block should be enabled on a per-board
basis, such that there should be a status = "disabled" property here by
default.
-- 
Florian

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/02/2017 11:14 PM, Florian Fainelli wrote:
> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> Northstar devices have MDIO bus that may contain various PHYs attached.
>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>> index acee36a61004..6a2afe7880ae 100644
>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>> @@ -320,6 +320,13 @@
>>  		};
>>  	};
>>
>> +	mdio@18003000 {
>> +		compatible = "brcm,iproc-mdio";
>> +		reg = <0x18003000 0x8>;
>> +		#size-cells = <1>;
>> +		#address-cells = <0>;
>> +	};
>
> This looks fine, but usually the block should be enabled on a per-board
> basis, such that there should be a status = "disabled" property here by
> default.

I think we have few blocks in bcm5301x.dtsi enabled by default. I guess it's
for stuff that is always present on every SoC family board: rng, nand, spi to
name few.

It makes some sense, consider e.g. spi. Every Northstar board has SPI
controller so it's enabled by default. Not every board has SPI flash, so it's
disabled by default.

It's there and it make sense to me. Is that OK or not?

I find MDIO situation quite simiar. It seems every Northstar board has MDIO bus
just devices may differ and should not be enabled by default.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/02/2017 11:14 PM, Florian Fainelli wrote:
> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>
>> Northstar devices have MDIO bus that may contain various PHYs attached.
>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>
>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>> ---
>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
>> index acee36a61004..6a2afe7880ae 100644
>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>> @@ -320,6 +320,13 @@
>>  		};
>>  	};
>>
>> +	mdio at 18003000 {
>> +		compatible = "brcm,iproc-mdio";
>> +		reg = <0x18003000 0x8>;
>> +		#size-cells = <1>;
>> +		#address-cells = <0>;
>> +	};
>
> This looks fine, but usually the block should be enabled on a per-board
> basis, such that there should be a status = "disabled" property here by
> default.

I think we have few blocks in bcm5301x.dtsi enabled by default. I guess it's
for stuff that is always present on every SoC family board: rng, nand, spi to
name few.

It makes some sense, consider e.g. spi. Every Northstar board has SPI
controller so it's enabled by default. Not every board has SPI flash, so it's
disabled by default.

It's there and it make sense to me. Is that OK or not?

I find MDIO situation quite simiar. It seems every Northstar board has MDIO bus
just devices may differ and should not be enabled by default.

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>
>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>
>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>> ---
>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index acee36a61004..6a2afe7880ae 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -320,6 +320,13 @@
>>>          };
>>>      };
>>>
>>> +    mdio@18003000 {
>>> +        compatible = "brcm,iproc-mdio";
>>> +        reg = <0x18003000 0x8>;
>>> +        #size-cells = <1>;
>>> +        #address-cells = <0>;
>>> +    };
>>
>> This looks fine, but usually the block should be enabled on a per-board
>> basis, such that there should be a status = "disabled" property here by
>> default.
> 
> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
> it's
> for stuff that is always present on every SoC family board: rng, nand,
> spi to
> name few.
> 
> It makes some sense, consider e.g. spi. Every Northstar board has SPI
> controller so it's enabled by default. Not every board has SPI flash, so
> it's
> disabled by default.
> 
> It's there and it make sense to me. Is that OK or not?

Even though there are devices that are always enabled on a given SoC,
because the board designs are always consistent does not necessarily
make them good candidates to be enabled at the .dtsi level. This is
particularly true when there are external connections to blocks (SPI,
NAND, USB, Ethernet, MDIO to name a few), having them disabled by
default is safer as a starting point to begin with.

> 
> I find MDIO situation quite simiar. It seems every Northstar board has
> MDIO bus
> just devices may differ and should not be enabled by default.

In which case, the only difference, for you would be to do to, at the
board-level DTS:

&mdio {
	status = "okay";

	phy@0 {
		reg = <0>;
		...
	};
};

versus:

&mdio {
	phy@0 {
		reg = <0>;
		...
	};
};

I think we can afford putting the mdio node's status property in each
board-level DTS and make it clear that way that it is enabled because
there are child nodes enabled?

NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
devices because it relies on child nodes being declared, but you would
still get the driver to be probed and enabled, which is a waste of
resources at best.

Thanks
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>
>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>
>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>> ---
>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>> index acee36a61004..6a2afe7880ae 100644
>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>> @@ -320,6 +320,13 @@
>>>          };
>>>      };
>>>
>>> +    mdio at 18003000 {
>>> +        compatible = "brcm,iproc-mdio";
>>> +        reg = <0x18003000 0x8>;
>>> +        #size-cells = <1>;
>>> +        #address-cells = <0>;
>>> +    };
>>
>> This looks fine, but usually the block should be enabled on a per-board
>> basis, such that there should be a status = "disabled" property here by
>> default.
> 
> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
> it's
> for stuff that is always present on every SoC family board: rng, nand,
> spi to
> name few.
> 
> It makes some sense, consider e.g. spi. Every Northstar board has SPI
> controller so it's enabled by default. Not every board has SPI flash, so
> it's
> disabled by default.
> 
> It's there and it make sense to me. Is that OK or not?

Even though there are devices that are always enabled on a given SoC,
because the board designs are always consistent does not necessarily
make them good candidates to be enabled at the .dtsi level. This is
particularly true when there are external connections to blocks (SPI,
NAND, USB, Ethernet, MDIO to name a few), having them disabled by
default is safer as a starting point to begin with.

> 
> I find MDIO situation quite simiar. It seems every Northstar board has
> MDIO bus
> just devices may differ and should not be enabled by default.

In which case, the only difference, for you would be to do to, at the
board-level DTS:

&mdio {
	status = "okay";

	phy at 0 {
		reg = <0>;
		...
	};
};

versus:

&mdio {
	phy at 0 {
		reg = <0>;
		...
	};
};

I think we can afford putting the mdio node's status property in each
board-level DTS and make it clear that way that it is enabled because
there are child nodes enabled?

NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
devices because it relies on child nodes being declared, but you would
still get the driver to be probed and enabled, which is a waste of
resources at best.

Thanks
-- 
Florian

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/19/2017 06:43 PM, Florian Fainelli wrote:
> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>> ---
>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> index acee36a61004..6a2afe7880ae 100644
>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> @@ -320,6 +320,13 @@
>>>>          };
>>>>      };
>>>>
>>>> +    mdio@18003000 {
>>>> +        compatible = "brcm,iproc-mdio";
>>>> +        reg = <0x18003000 0x8>;
>>>> +        #size-cells = <1>;
>>>> +        #address-cells = <0>;
>>>> +    };
>>>
>>> This looks fine, but usually the block should be enabled on a per-board
>>> basis, such that there should be a status = "disabled" property here by
>>> default.
>>
>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>> it's
>> for stuff that is always present on every SoC family board: rng, nand,
>> spi to
>> name few.
>>
>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>> controller so it's enabled by default. Not every board has SPI flash, so
>> it's
>> disabled by default.
>>
>> It's there and it make sense to me. Is that OK or not?
>
> Even though there are devices that are always enabled on a given SoC,
> because the board designs are always consistent does not necessarily
> make them good candidates to be enabled at the .dtsi level. This is
> particularly true when there are external connections to blocks (SPI,
> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
> default is safer as a starting point to begin with.

In case of Northstar there is USB 3.0 PHY connected *internally* to this MDIO.
I don't think any board manufacturer is able to rip SoC out of the MDIO or the
USB 3.0 PHY.


>> I find MDIO situation quite simiar. It seems every Northstar board has
>> MDIO bus
>> just devices may differ and should not be enabled by default.
>
> In which case, the only difference, for you would be to do to, at the
> board-level DTS:
>
> &mdio {
> 	status = "okay";
>
> 	phy@0 {
> 		reg = <0>;
> 		...
> 	};
> };
>
> versus:
>
> &mdio {
> 	phy@0 {
> 		reg = <0>;
> 		...
> 	};
> };
>
> I think we can afford putting the mdio node's status property in each
> board-level DTS and make it clear that way that it is enabled because
> there are child nodes enabled?

This will be a pretty big effort because every Northstar device I know has USB
3.0 PHY in the SoC.


> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
> devices because it relies on child nodes being declared, but you would
> still get the driver to be probed and enabled, which is a waste of
> resources at best.

Right, but DT role is to describe device/board and not really care if operating
system handles that efficiently.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/19/2017 06:43 PM, Florian Fainelli wrote:
> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>
>>>> Northstar devices have MDIO bus that may contain various PHYs attached.
>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).
>>>>
>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>> ---
>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> index acee36a61004..6a2afe7880ae 100644
>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>> @@ -320,6 +320,13 @@
>>>>          };
>>>>      };
>>>>
>>>> +    mdio at 18003000 {
>>>> +        compatible = "brcm,iproc-mdio";
>>>> +        reg = <0x18003000 0x8>;
>>>> +        #size-cells = <1>;
>>>> +        #address-cells = <0>;
>>>> +    };
>>>
>>> This looks fine, but usually the block should be enabled on a per-board
>>> basis, such that there should be a status = "disabled" property here by
>>> default.
>>
>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>> it's
>> for stuff that is always present on every SoC family board: rng, nand,
>> spi to
>> name few.
>>
>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>> controller so it's enabled by default. Not every board has SPI flash, so
>> it's
>> disabled by default.
>>
>> It's there and it make sense to me. Is that OK or not?
>
> Even though there are devices that are always enabled on a given SoC,
> because the board designs are always consistent does not necessarily
> make them good candidates to be enabled at the .dtsi level. This is
> particularly true when there are external connections to blocks (SPI,
> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
> default is safer as a starting point to begin with.

In case of Northstar there is USB 3.0 PHY connected *internally* to this MDIO.
I don't think any board manufacturer is able to rip SoC out of the MDIO or the
USB 3.0 PHY.


>> I find MDIO situation quite simiar. It seems every Northstar board has
>> MDIO bus
>> just devices may differ and should not be enabled by default.
>
> In which case, the only difference, for you would be to do to, at the
> board-level DTS:
>
> &mdio {
> 	status = "okay";
>
> 	phy at 0 {
> 		reg = <0>;
> 		...
> 	};
> };
>
> versus:
>
> &mdio {
> 	phy at 0 {
> 		reg = <0>;
> 		...
> 	};
> };
>
> I think we can afford putting the mdio node's status property in each
> board-level DTS and make it clear that way that it is enabled because
> there are child nodes enabled?

This will be a pretty big effort because every Northstar device I know has USB
3.0 PHY in the SoC.


> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
> devices because it relies on child nodes being declared, but you would
> still get the driver to be probed and enabled, which is a waste of
> resources at best.

Right, but DT role is to describe device/board and not really care if operating
system handles that efficiently.

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>
>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>> attached.
>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>> yet).
>>>>>
>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>> ---
>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -320,6 +320,13 @@
>>>>>          };
>>>>>      };
>>>>>
>>>>> +    mdio@18003000 {
>>>>> +        compatible = "brcm,iproc-mdio";
>>>>> +        reg = <0x18003000 0x8>;
>>>>> +        #size-cells = <1>;
>>>>> +        #address-cells = <0>;
>>>>> +    };
>>>>
>>>> This looks fine, but usually the block should be enabled on a per-board
>>>> basis, such that there should be a status = "disabled" property here by
>>>> default.
>>>
>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>> it's
>>> for stuff that is always present on every SoC family board: rng, nand,
>>> spi to
>>> name few.
>>>
>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>> controller so it's enabled by default. Not every board has SPI flash, so
>>> it's
>>> disabled by default.
>>>
>>> It's there and it make sense to me. Is that OK or not?
>>
>> Even though there are devices that are always enabled on a given SoC,
>> because the board designs are always consistent does not necessarily
>> make them good candidates to be enabled at the .dtsi level. This is
>> particularly true when there are external connections to blocks (SPI,
>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>> default is safer as a starting point to begin with.
> 
> In case of Northstar there is USB 3.0 PHY connected *internally* to this
> MDIO.
> I don't think any board manufacturer is able to rip SoC out of the MDIO
> or the
> USB 3.0 PHY.

OK, then can you still resubmit a proper patch that a) puts that
information in the commit message, and b) also adds a proper label to
the mdio node such that it can later on be referenced by label in
board-level DTS files? By that I mean:

mdio: mdio@18003000 {

Thank you

> 
> 
>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>> MDIO bus
>>> just devices may differ and should not be enabled by default.
>>
>> In which case, the only difference, for you would be to do to, at the
>> board-level DTS:
>>
>> &mdio {
>>     status = "okay";
>>
>>     phy@0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> versus:
>>
>> &mdio {
>>     phy@0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> I think we can afford putting the mdio node's status property in each
>> board-level DTS and make it clear that way that it is enabled because
>> there are child nodes enabled?
> 
> This will be a pretty big effort because every Northstar device I know
> has USB
> 3.0 PHY in the SoC.

Adding a one liner is a "pretty big effort", for sure.

> 
> 
>> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
>> devices because it relies on child nodes being declared, but you would
>> still get the driver to be probed and enabled, which is a waste of
>> resources at best.
> 
> Right, but DT role is to describe device/board and not really care if
> operating
> system handles that efficiently.


-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>
>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>> attached.
>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>> yet).
>>>>>
>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>> ---
>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>> @@ -320,6 +320,13 @@
>>>>>          };
>>>>>      };
>>>>>
>>>>> +    mdio at 18003000 {
>>>>> +        compatible = "brcm,iproc-mdio";
>>>>> +        reg = <0x18003000 0x8>;
>>>>> +        #size-cells = <1>;
>>>>> +        #address-cells = <0>;
>>>>> +    };
>>>>
>>>> This looks fine, but usually the block should be enabled on a per-board
>>>> basis, such that there should be a status = "disabled" property here by
>>>> default.
>>>
>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>> it's
>>> for stuff that is always present on every SoC family board: rng, nand,
>>> spi to
>>> name few.
>>>
>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>> controller so it's enabled by default. Not every board has SPI flash, so
>>> it's
>>> disabled by default.
>>>
>>> It's there and it make sense to me. Is that OK or not?
>>
>> Even though there are devices that are always enabled on a given SoC,
>> because the board designs are always consistent does not necessarily
>> make them good candidates to be enabled at the .dtsi level. This is
>> particularly true when there are external connections to blocks (SPI,
>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>> default is safer as a starting point to begin with.
> 
> In case of Northstar there is USB 3.0 PHY connected *internally* to this
> MDIO.
> I don't think any board manufacturer is able to rip SoC out of the MDIO
> or the
> USB 3.0 PHY.

OK, then can you still resubmit a proper patch that a) puts that
information in the commit message, and b) also adds a proper label to
the mdio node such that it can later on be referenced by label in
board-level DTS files? By that I mean:

mdio: mdio at 18003000 {

Thank you

> 
> 
>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>> MDIO bus
>>> just devices may differ and should not be enabled by default.
>>
>> In which case, the only difference, for you would be to do to, at the
>> board-level DTS:
>>
>> &mdio {
>>     status = "okay";
>>
>>     phy at 0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> versus:
>>
>> &mdio {
>>     phy at 0 {
>>         reg = <0>;
>>         ...
>>     };
>> };
>>
>> I think we can afford putting the mdio node's status property in each
>> board-level DTS and make it clear that way that it is enabled because
>> there are child nodes enabled?
> 
> This will be a pretty big effort because every Northstar device I know
> has USB
> 3.0 PHY in the SoC.

Adding a one liner is a "pretty big effort", for sure.

> 
> 
>> NB: with a CONFIG_OF system, there is no automatic probing of MDIO child
>> devices because it relies on child nodes being declared, but you would
>> still get the driver to be probed and enabled, which is a waste of
>> resources at best.
> 
> Right, but DT role is to describe device/board and not really care if
> operating
> system handles that efficiently.


-- 
Florian

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/19/2017 07:52 PM, Florian Fainelli wrote:
> On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>>>
>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>> attached.
>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>> yet).
>>>>>>
>>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> @@ -320,6 +320,13 @@
>>>>>>          };
>>>>>>      };
>>>>>>
>>>>>> +    mdio@18003000 {
>>>>>> +        compatible = "brcm,iproc-mdio";
>>>>>> +        reg = <0x18003000 0x8>;
>>>>>> +        #size-cells = <1>;
>>>>>> +        #address-cells = <0>;
>>>>>> +    };
>>>>>
>>>>> This looks fine, but usually the block should be enabled on a per-board
>>>>> basis, such that there should be a status = "disabled" property here by
>>>>> default.
>>>>
>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>>> it's
>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>> spi to
>>>> name few.
>>>>
>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>> controller so it's enabled by default. Not every board has SPI flash, so
>>>> it's
>>>> disabled by default.
>>>>
>>>> It's there and it make sense to me. Is that OK or not?
>>>
>>> Even though there are devices that are always enabled on a given SoC,
>>> because the board designs are always consistent does not necessarily
>>> make them good candidates to be enabled at the .dtsi level. This is
>>> particularly true when there are external connections to blocks (SPI,
>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>> default is safer as a starting point to begin with.
>>
>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>> MDIO.
>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>> or the
>> USB 3.0 PHY.
>
> OK, then can you still resubmit a proper patch that a) puts that
> information in the commit message, and b) also adds a proper label to
> the mdio node such that it can later on be referenced by label in
> board-level DTS files? By that I mean:
>
> mdio: mdio@18003000 {
>
> Thank you
>
>>
>>
>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>> MDIO bus
>>>> just devices may differ and should not be enabled by default.
>>>
>>> In which case, the only difference, for you would be to do to, at the
>>> board-level DTS:
>>>
>>> &mdio {
>>>     status = "okay";
>>>
>>>     phy@0 {
>>>         reg = <0>;
>>>         ...
>>>     };
>>> };
>>>
>>> versus:
>>>
>>> &mdio {
>>>     phy@0 {
>>>         reg = <0>;
>>>         ...
>>>     };
>>> };
>>>
>>> I think we can afford putting the mdio node's status property in each
>>> board-level DTS and make it clear that way that it is enabled because
>>> there are child nodes enabled?
>>
>> This will be a pretty big effort because every Northstar device I know
>> has USB
>> 3.0 PHY in the SoC.
>
> Adding a one liner is a "pretty big effort", for sure.

Sorry, we got a misunderstanding here.

I thought you meant adding something like this for every device:

&mdio {
	status = "okay";

	usb3_phy: usb-phy@10 {
		compatible = "brcm,ns-ax-usb3-phy";
		reg = <0x10>;
		usb3-dmp-syscon = <&usb3_dmp>;
		#phy-cells = <0>;
	};
};

usb3_dmp: syscon@18105000 {
	reg = <0x18105000 0x1000>;
};

So I clearly missed something important. Did you want to have USB 3.0 PHY
defined in the dtsi file?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

On 04/19/2017 07:52 PM, Florian Fainelli wrote:
> On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>
>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>> attached.
>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>> yet).
>>>>>>
>>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>  1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>> @@ -320,6 +320,13 @@
>>>>>>          };
>>>>>>      };
>>>>>>
>>>>>> +    mdio at 18003000 {
>>>>>> +        compatible = "brcm,iproc-mdio";
>>>>>> +        reg = <0x18003000 0x8>;
>>>>>> +        #size-cells = <1>;
>>>>>> +        #address-cells = <0>;
>>>>>> +    };
>>>>>
>>>>> This looks fine, but usually the block should be enabled on a per-board
>>>>> basis, such that there should be a status = "disabled" property here by
>>>>> default.
>>>>
>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I guess
>>>> it's
>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>> spi to
>>>> name few.
>>>>
>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>> controller so it's enabled by default. Not every board has SPI flash, so
>>>> it's
>>>> disabled by default.
>>>>
>>>> It's there and it make sense to me. Is that OK or not?
>>>
>>> Even though there are devices that are always enabled on a given SoC,
>>> because the board designs are always consistent does not necessarily
>>> make them good candidates to be enabled at the .dtsi level. This is
>>> particularly true when there are external connections to blocks (SPI,
>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>> default is safer as a starting point to begin with.
>>
>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>> MDIO.
>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>> or the
>> USB 3.0 PHY.
>
> OK, then can you still resubmit a proper patch that a) puts that
> information in the commit message, and b) also adds a proper label to
> the mdio node such that it can later on be referenced by label in
> board-level DTS files? By that I mean:
>
> mdio: mdio at 18003000 {
>
> Thank you
>
>>
>>
>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>> MDIO bus
>>>> just devices may differ and should not be enabled by default.
>>>
>>> In which case, the only difference, for you would be to do to, at the
>>> board-level DTS:
>>>
>>> &mdio {
>>>     status = "okay";
>>>
>>>     phy at 0 {
>>>         reg = <0>;
>>>         ...
>>>     };
>>> };
>>>
>>> versus:
>>>
>>> &mdio {
>>>     phy at 0 {
>>>         reg = <0>;
>>>         ...
>>>     };
>>> };
>>>
>>> I think we can afford putting the mdio node's status property in each
>>> board-level DTS and make it clear that way that it is enabled because
>>> there are child nodes enabled?
>>
>> This will be a pretty big effort because every Northstar device I know
>> has USB
>> 3.0 PHY in the SoC.
>
> Adding a one liner is a "pretty big effort", for sure.

Sorry, we got a misunderstanding here.

I thought you meant adding something like this for every device:

&mdio {
	status = "okay";

	usb3_phy: usb-phy at 10 {
		compatible = "brcm,ns-ax-usb3-phy";
		reg = <0x10>;
		usb3-dmp-syscon = <&usb3_dmp>;
		#phy-cells = <0>;
	};
};

usb3_dmp: syscon at 18105000 {
	reg = <0x18105000 0x1000>;
};

So I clearly missed something important. Did you want to have USB 3.0 PHY
defined in the dtsi file?

Re: [PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/19/2017 11:10 AM, Rafał Miłecki wrote:
> On 04/19/2017 07:52 PM, Florian Fainelli wrote:
>> On 04/19/2017 10:35 AM, Rafał Miłecki wrote:
>>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>>> On 04/02/2017 02:25 PM, Rafał Miłecki wrote:
>>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>>> Le 04/02/17 à 14:08, Rafał Miłecki a écrit :
>>>>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>>
>>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>>> attached.
>>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>>> yet).
>>>>>>>
>>>>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> @@ -320,6 +320,13 @@
>>>>>>>          };
>>>>>>>      };
>>>>>>>
>>>>>>> +    mdio@18003000 {
>>>>>>> +        compatible = "brcm,iproc-mdio";
>>>>>>> +        reg = <0x18003000 0x8>;
>>>>>>> +        #size-cells = <1>;
>>>>>>> +        #address-cells = <0>;
>>>>>>> +    };
>>>>>>
>>>>>> This looks fine, but usually the block should be enabled on a
>>>>>> per-board
>>>>>> basis, such that there should be a status = "disabled" property
>>>>>> here by
>>>>>> default.
>>>>>
>>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I
>>>>> guess
>>>>> it's
>>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>>> spi to
>>>>> name few.
>>>>>
>>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>>> controller so it's enabled by default. Not every board has SPI
>>>>> flash, so
>>>>> it's
>>>>> disabled by default.
>>>>>
>>>>> It's there and it make sense to me. Is that OK or not?
>>>>
>>>> Even though there are devices that are always enabled on a given SoC,
>>>> because the board designs are always consistent does not necessarily
>>>> make them good candidates to be enabled at the .dtsi level. This is
>>>> particularly true when there are external connections to blocks (SPI,
>>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>>> default is safer as a starting point to begin with.
>>>
>>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>>> MDIO.
>>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>>> or the
>>> USB 3.0 PHY.
>>
>> OK, then can you still resubmit a proper patch that a) puts that
>> information in the commit message, and b) also adds a proper label to
>> the mdio node such that it can later on be referenced by label in
>> board-level DTS files? By that I mean:
>>
>> mdio: mdio@18003000 {
>>
>> Thank you
>>
>>>
>>>
>>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>>> MDIO bus
>>>>> just devices may differ and should not be enabled by default.
>>>>
>>>> In which case, the only difference, for you would be to do to, at the
>>>> board-level DTS:
>>>>
>>>> &mdio {
>>>>     status = "okay";
>>>>
>>>>     phy@0 {
>>>>         reg = <0>;
>>>>         ...
>>>>     };
>>>> };
>>>>
>>>> versus:
>>>>
>>>> &mdio {
>>>>     phy@0 {
>>>>         reg = <0>;
>>>>         ...
>>>>     };
>>>> };
>>>>
>>>> I think we can afford putting the mdio node's status property in each
>>>> board-level DTS and make it clear that way that it is enabled because
>>>> there are child nodes enabled?
>>>
>>> This will be a pretty big effort because every Northstar device I know
>>> has USB
>>> 3.0 PHY in the SoC.
>>
>> Adding a one liner is a "pretty big effort", for sure.
> 
> Sorry, we got a misunderstanding here.
> 
> I thought you meant adding something like this for every device:
> 
> &mdio {
>     status = "okay";
> 
>     usb3_phy: usb-phy@10 {
>         compatible = "brcm,ns-ax-usb3-phy";
>         reg = <0x10>;
>         usb3-dmp-syscon = <&usb3_dmp>;
>         #phy-cells = <0>;
>     };
> };

Ah no, I would have just done the following in the per-board DTS:

&mdio {
	status = "okay";
};

&usb3_phy {
	status = "okay";
};

&xhci {
	status = "okay";
};

Something like that.

> 
> usb3_dmp: syscon@18105000 {
>     reg = <0x18105000 0x1000>;
> };
> 
> So I clearly missed something important. Did you want to have USB 3.0 PHY
> defined in the dtsi file?

Yes, I think it does make sense to have it defined in the .dtsi file
because it's internal to the SoC, however it should probably be marked
disabled by default, unless a board enables its xHCI controller, does
that make sense?
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Florian Fainelli]

On 04/19/2017 11:10 AM, Rafa? Mi?ecki wrote:
> On 04/19/2017 07:52 PM, Florian Fainelli wrote:
>> On 04/19/2017 10:35 AM, Rafa? Mi?ecki wrote:
>>> On 04/19/2017 06:43 PM, Florian Fainelli wrote:
>>>> On 04/02/2017 02:25 PM, Rafa? Mi?ecki wrote:
>>>>> On 04/02/2017 11:14 PM, Florian Fainelli wrote:
>>>>>> Le 04/02/17 ? 14:08, Rafa? Mi?ecki a ?crit :
>>>>>>> From: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>>
>>>>>>> Northstar devices have MDIO bus that may contain various PHYs
>>>>>>> attached.
>>>>>>> A common example is USB 3.0 PHY (that doesn't have an MDIO driver
>>>>>>> yet).
>>>>>>>
>>>>>>> Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/bcm5301x.dtsi | 7 +++++++
>>>>>>>  1 file changed, 7 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> index acee36a61004..6a2afe7880ae 100644
>>>>>>> --- a/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/bcm5301x.dtsi
>>>>>>> @@ -320,6 +320,13 @@
>>>>>>>          };
>>>>>>>      };
>>>>>>>
>>>>>>> +    mdio at 18003000 {
>>>>>>> +        compatible = "brcm,iproc-mdio";
>>>>>>> +        reg = <0x18003000 0x8>;
>>>>>>> +        #size-cells = <1>;
>>>>>>> +        #address-cells = <0>;
>>>>>>> +    };
>>>>>>
>>>>>> This looks fine, but usually the block should be enabled on a
>>>>>> per-board
>>>>>> basis, such that there should be a status = "disabled" property
>>>>>> here by
>>>>>> default.
>>>>>
>>>>> I think we have few blocks in bcm5301x.dtsi enabled by default. I
>>>>> guess
>>>>> it's
>>>>> for stuff that is always present on every SoC family board: rng, nand,
>>>>> spi to
>>>>> name few.
>>>>>
>>>>> It makes some sense, consider e.g. spi. Every Northstar board has SPI
>>>>> controller so it's enabled by default. Not every board has SPI
>>>>> flash, so
>>>>> it's
>>>>> disabled by default.
>>>>>
>>>>> It's there and it make sense to me. Is that OK or not?
>>>>
>>>> Even though there are devices that are always enabled on a given SoC,
>>>> because the board designs are always consistent does not necessarily
>>>> make them good candidates to be enabled at the .dtsi level. This is
>>>> particularly true when there are external connections to blocks (SPI,
>>>> NAND, USB, Ethernet, MDIO to name a few), having them disabled by
>>>> default is safer as a starting point to begin with.
>>>
>>> In case of Northstar there is USB 3.0 PHY connected *internally* to this
>>> MDIO.
>>> I don't think any board manufacturer is able to rip SoC out of the MDIO
>>> or the
>>> USB 3.0 PHY.
>>
>> OK, then can you still resubmit a proper patch that a) puts that
>> information in the commit message, and b) also adds a proper label to
>> the mdio node such that it can later on be referenced by label in
>> board-level DTS files? By that I mean:
>>
>> mdio: mdio at 18003000 {
>>
>> Thank you
>>
>>>
>>>
>>>>> I find MDIO situation quite simiar. It seems every Northstar board has
>>>>> MDIO bus
>>>>> just devices may differ and should not be enabled by default.
>>>>
>>>> In which case, the only difference, for you would be to do to, at the
>>>> board-level DTS:
>>>>
>>>> &mdio {
>>>>     status = "okay";
>>>>
>>>>     phy at 0 {
>>>>         reg = <0>;
>>>>         ...
>>>>     };
>>>> };
>>>>
>>>> versus:
>>>>
>>>> &mdio {
>>>>     phy at 0 {
>>>>         reg = <0>;
>>>>         ...
>>>>     };
>>>> };
>>>>
>>>> I think we can afford putting the mdio node's status property in each
>>>> board-level DTS and make it clear that way that it is enabled because
>>>> there are child nodes enabled?
>>>
>>> This will be a pretty big effort because every Northstar device I know
>>> has USB
>>> 3.0 PHY in the SoC.
>>
>> Adding a one liner is a "pretty big effort", for sure.
> 
> Sorry, we got a misunderstanding here.
> 
> I thought you meant adding something like this for every device:
> 
> &mdio {
>     status = "okay";
> 
>     usb3_phy: usb-phy at 10 {
>         compatible = "brcm,ns-ax-usb3-phy";
>         reg = <0x10>;
>         usb3-dmp-syscon = <&usb3_dmp>;
>         #phy-cells = <0>;
>     };
> };

Ah no, I would have just done the following in the per-board DTS:

&mdio {
	status = "okay";
};

&usb3_phy {
	status = "okay";
};

&xhci {
	status = "okay";
};

Something like that.

> 
> usb3_dmp: syscon at 18105000 {
>     reg = <0x18105000 0x1000>;
> };
> 
> So I clearly missed something important. Did you want to have USB 3.0 PHY
> defined in the dtsi file?

Yes, I think it does make sense to have it defined in the .dtsi file
because it's internal to the SoC, however it should probably be marked
disabled by default, unless a board enables its xHCI controller, does
that make sense?
-- 
Florian

[PATCH V2] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Northstar devices have MDIO bus that may contain various PHYs attached.
A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
V2: Disable MDIO bus by default.
    Add mdio label to allow enabling it easily per board
---
 arch/arm/boot/dts/bcm5301x.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index acee36a61004..1f34bc6905f2 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -320,6 +320,14 @@
 		};
 	};
 
+	mdio: mdio@18003000 {
+		compatible = "brcm,iproc-mdio";
+		reg = <0x18003000 0x8>;
+		#size-cells = <1>;
+		#address-cells = <0>;
+		status = "disabled";
+	};
+
 	i2c0: i2c@18009000 {
 		compatible = "brcm,iproc-i2c";
 		reg = <0x18009000 0x50>;
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] ARM: dts: BCM5301X: Specify MDIO bus in the DT

[Rafał Miłecki]

From: Rafa? Mi?ecki <rafal@milecki.pl>

Northstar devices have MDIO bus that may contain various PHYs attached.
A common example is USB 3.0 PHY (that doesn't have an MDIO driver yet).

Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl>
---
V2: Disable MDIO bus by default.
    Add mdio label to allow enabling it easily per board
---
 arch/arm/boot/dts/bcm5301x.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/bcm5301x.dtsi b/arch/arm/boot/dts/bcm5301x.dtsi
index acee36a61004..1f34bc6905f2 100644
--- a/arch/arm/boot/dts/bcm5301x.dtsi
+++ b/arch/arm/boot/dts/bcm5301x.dtsi
@@ -320,6 +320,14 @@
 		};
 	};
 
+	mdio: mdio at 18003000 {
+		compatible = "brcm,iproc-mdio";
+		reg = <0x18003000 0x8>;
+		#size-cells = <1>;
+		#address-cells = <0>;
+		status = "disabled";
+	};
+
 	i2c0: i2c at 18009000 {
 		compatible = "brcm,iproc-i2c";
 		reg = <0x18009000 0x50>;
-- 
2.11.0