[linux,dev-4.10] aspeed_g5_defconfig: add CONFIG_OF_DYNAMIC.

Message ID 20170803144813.28010-1-c_mykolak@mellanox.com
State Rejected, archived
Headers show

Commit Message

Mykola Kostenok Aug. 3, 2017, 2:48 p.m.
Add CONFIG_OF_DYNAMIC for hotplug operations.
It requires CONFIG_OF_UNITTEST so it added too.

Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com>
---
 arch/arm/configs/aspeed_g5_defconfig | 2 ++
 1 file changed, 2 insertions(+)

Comments

Joel Stanley Aug. 8, 2017, 1:08 p.m. | #1
Hey Mykola,

On Fri, Aug 4, 2017 at 12:48 AM, Mykola Kostenok <c_mykolak@mellanox.com> wrote:
> Add CONFIG_OF_DYNAMIC for hotplug operations.
> It requires CONFIG_OF_UNITTEST so it added too.
>
> Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com>
> ---
>  arch/arm/configs/aspeed_g5_defconfig | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
> index 2025e45..e78e306 100644
> --- a/arch/arm/configs/aspeed_g5_defconfig
> +++ b/arch/arm/configs/aspeed_g5_defconfig
> @@ -215,3 +215,5 @@ CONFIG_DEBUG_USER=y
>  # CONFIG_XZ_DEC_POWERPC is not set
>  # CONFIG_XZ_DEC_IA64 is not set
>  # CONFIG_XZ_DEC_SPARC is not set
> +CONFIG_OF_UNITTEST=y
> +CONFIG_OF_DYNAMIC=y

I'm not sure that this is something we want enabled:

CONFIG_OF_UNITTEST:
 This option builds in test cases for the device tree infrastructure
 that are executed once at boot time, and the results dumped to the
 console.

CONFIG_OF_DYNAMIC:
 On some platforms, the device tree can be manipulated at runtime.
 While this option is selected automatically on such platforms, you
 can enable it manually to improve device tree unit test coverage.

Can you explain what you're wanting to do here?

I've added some of our device tree experts to cc so they can offer advice.

Cheers,

Joel
Mykola Kostenok Aug. 8, 2017, 2:26 p.m. | #2
> -----Original Message-----

> From: joel.stan@gmail.com [mailto:joel.stan@gmail.com] On Behalf Of Joel

> Stanley

> Sent: Tuesday, August 8, 2017 4:09 PM

> To: Mykola Kostenok <c_mykolak@mellanox.com>

> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>; Vadim Pasternak

> <vadimp@mellanox.com>; Ohad Oz <ohado@mellanox.com>; Benjamin

> Herrenschmidt <benh@kernel.crashing.org>; Michael Ellerman

> <mpe@ellerman.id.au>; Jeremy Kerr <jk@ozlabs.org>

> Subject: Re: [PATCH linux dev-4.10] aspeed_g5_defconfig: add

> CONFIG_OF_DYNAMIC.

> 

> Hey Mykola,

> 

> On Fri, Aug 4, 2017 at 12:48 AM, Mykola Kostenok

> <c_mykolak@mellanox.com> wrote:

> > Add CONFIG_OF_DYNAMIC for hotplug operations.

> > It requires CONFIG_OF_UNITTEST so it added too.

> >

> > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com>

> > ---

> >  arch/arm/configs/aspeed_g5_defconfig | 2 ++

> >  1 file changed, 2 insertions(+)

> >

> > diff --git a/arch/arm/configs/aspeed_g5_defconfig

> > b/arch/arm/configs/aspeed_g5_defconfig

> > index 2025e45..e78e306 100644

> > --- a/arch/arm/configs/aspeed_g5_defconfig

> > +++ b/arch/arm/configs/aspeed_g5_defconfig

> > @@ -215,3 +215,5 @@ CONFIG_DEBUG_USER=y  #

> CONFIG_XZ_DEC_POWERPC is

> > not set  # CONFIG_XZ_DEC_IA64 is not set  # CONFIG_XZ_DEC_SPARC is

> not

> > set

> > +CONFIG_OF_UNITTEST=y

> > +CONFIG_OF_DYNAMIC=y

> 

> I'm not sure that this is something we want enabled:

> 

> CONFIG_OF_UNITTEST:

>  This option builds in test cases for the device tree infrastructure  that are

> executed once at boot time, and the results dumped to the  console.

> 

> CONFIG_OF_DYNAMIC:

>  On some platforms, the device tree can be manipulated at runtime.

>  While this option is selected automatically on such platforms, you  can

> enable it manually to improve device tree unit test coverage.

> 

> Can you explain what you're wanting to do here?

> 

> I've added some of our device tree experts to cc so they can offer advice.

> 

> Cheers,

> 

> Joel


Hi, Joel.

We have now patchset supporting our CPLD device in upstream review.
There we have support for dynamic (hotplug) devices, like PSUs, FANs, ASIC, etc.  

These devices are described in DTS like, f.e. 
&i2c3 {
	status = "okay";
	#address-cells = <1>;
	#size-cells = <0>;

	psu1_eeprom: eeprom@51 {
		compatible = "atmel,24c02";
		reg = <0x51>;
		status = "disabled";
	};

	psu2_eeprom: eeprom@50 {
		compatible = "atmel,24c02";
		reg = <0x50>;
		status = "disabled";
	};

	psu1_ctrl: dps460@59 {
		compatible = "dps460";
		reg = <0x59>;
		status = "disabled";
	};

	psu2_ctrl: dps460@58 {
		compatible = "dps460";
		reg = <0x58>;
		status = "disabled";
	};
};

&i2c12 {
	status = "okay";
	#address-cells = <1>;
	#size-cells = <0>;

	asic_thermal: mlxsw_minimal@48 {
		compatible = "mlxsw_minimal";
		reg = <0x48>;
		status = "disabled";
		cooling-phandle = <&cooling>;

		trips {
			/* trip type, temp in mcelsius, temp min, temp max */
			trip@0 {
				trip = <0 75000 0 0>;
			};
			trip@1 {
				trip = <2 85000 1 5>;
			};
			trip@3 {
				trip = <2 105000 5 5>;
			};
		};
	};
};

They all sets with the state "disabled", and driver activates it with the below calls, when related hotplug event is received in interrupt handler:

static struct property mlxreg_core_device_dis = {
	.name = MLXREG_CORE_PROP_STATUS,
	.value = MLXREG_CORE_PROP_DISABLED,
	.length = sizeof(MLXREG_CORE_PROP_DISABLED),
};

static void mlxreg_core_dev_enable(struct device_node *np) {
	/* Enable and create device. */
	of_update_property(np, &mlxreg_core_device_en); }

static void mlxreg_core_dev_disable(struct device_node *np) {
	/* Disable and unregister platform device. */
	of_update_property(np, &mlxreg_core_device_dis);
	of_node_clear_flag(np, OF_POPULATED);
}

For that we must have OF_DYNAMIC set, and last one required OF_UNITEST.

Best regards. Mykola Kostenok.
Michael Ellerman Aug. 9, 2017, 1 p.m. | #3
Mykola Kostenok <c_mykolak@mellanox.com> writes:
>> -----Original Message-----
>> From: joel.stan@gmail.com [mailto:joel.stan@gmail.com] On Behalf Of Joel >> Stanley
>> 
>> Can you explain what you're wanting to do here?
>> 
>> I've added some of our device tree experts to cc so they can offer advice.
>
> Hi, Joel.
>
> We have now patchset supporting our CPLD device in upstream review.
> There we have support for dynamic (hotplug) devices, like PSUs, FANs, ASIC, etc.  
>
...
>
> They all sets with the state "disabled", and driver activates it with the below calls, when related hotplug event is received in interrupt handler:
>
> static struct property mlxreg_core_device_dis = {
> 	.name = MLXREG_CORE_PROP_STATUS,
> 	.value = MLXREG_CORE_PROP_DISABLED,
> 	.length = sizeof(MLXREG_CORE_PROP_DISABLED),
> };
>
> static void mlxreg_core_dev_enable(struct device_node *np) {
> 	/* Enable and create device. */
> 	of_update_property(np, &mlxreg_core_device_en); }
>
> static void mlxreg_core_dev_disable(struct device_node *np) {
> 	/* Disable and unregister platform device. */
> 	of_update_property(np, &mlxreg_core_device_dis);
> 	of_node_clear_flag(np, OF_POPULATED);
> }
>
> For that we must have OF_DYNAMIC set, and last one required OF_UNITEST.

But are you actually changing the structure of the device tree at run time?

If all you're doing is using of_update_property() to change the status
property I don't think you need OF_DYNAMIC.

cheers
Mykola Kostenok Aug. 9, 2017, 3:09 p.m. | #4
> -----Original Message-----
> From: Michael Ellerman [mailto:mpe@ellerman.id.au]
> Sent: Wednesday, August 9, 2017 4:01 PM
> To: Mykola Kostenok <c_mykolak@mellanox.com>; Joel Stanley
> <joel@jms.id.au>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>; Vadim Pasternak
> <vadimp@mellanox.com>; Ohad Oz <ohado@mellanox.com>; Benjamin
> Herrenschmidt <benh@kernel.crashing.org>; Jeremy Kerr <jk@ozlabs.org>
> Subject: RE: [PATCH linux dev-4.10] aspeed_g5_defconfig: add
> CONFIG_OF_DYNAMIC.
> 
> Mykola Kostenok <c_mykolak@mellanox.com> writes:
> >> -----Original Message-----
> >> From: joel.stan@gmail.com [mailto:joel.stan@gmail.com] On Behalf Of
> >> Joel >> Stanley
> >>
> >> Can you explain what you're wanting to do here?
> >>
> >> I've added some of our device tree experts to cc so they can offer advice.
> >
> > Hi, Joel.
> >
> > We have now patchset supporting our CPLD device in upstream review.
> > There we have support for dynamic (hotplug) devices, like PSUs, FANs,
> ASIC, etc.
> >
> ...
> >
> > They all sets with the state "disabled", and driver activates it with the
> below calls, when related hotplug event is received in interrupt handler:
> >
> > static struct property mlxreg_core_device_dis = {
> > 	.name = MLXREG_CORE_PROP_STATUS,
> > 	.value = MLXREG_CORE_PROP_DISABLED,
> > 	.length = sizeof(MLXREG_CORE_PROP_DISABLED),
> > };
> >
> > static void mlxreg_core_dev_enable(struct device_node *np) {
> > 	/* Enable and create device. */
> > 	of_update_property(np, &mlxreg_core_device_en); }
> >
> > static void mlxreg_core_dev_disable(struct device_node *np) {
> > 	/* Disable and unregister platform device. */
> > 	of_update_property(np, &mlxreg_core_device_dis);
> > 	of_node_clear_flag(np, OF_POPULATED); }
> >
> > For that we must have OF_DYNAMIC set, and last one required
> OF_UNITEST.
> 
> But are you actually changing the structure of the device tree at run time?
> 
> If all you're doing is using of_update_property() to change the status
> property I don't think you need OF_DYNAMIC.
> 
> cheers

Hi, Michael.

In drivers/i2c/i2c-core.c:

#if IS_ENABLED(CONFIG_OF_DYNAMIC)
static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
			 void *arg)
So we need it.

Best regards. Mykola Kostenok.

Patch

diff --git a/arch/arm/configs/aspeed_g5_defconfig b/arch/arm/configs/aspeed_g5_defconfig
index 2025e45..e78e306 100644
--- a/arch/arm/configs/aspeed_g5_defconfig
+++ b/arch/arm/configs/aspeed_g5_defconfig
@@ -215,3 +215,5 @@  CONFIG_DEBUG_USER=y
 # CONFIG_XZ_DEC_POWERPC is not set
 # CONFIG_XZ_DEC_IA64 is not set
 # CONFIG_XZ_DEC_SPARC is not set
+CONFIG_OF_UNITTEST=y
+CONFIG_OF_DYNAMIC=y