Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-27 Thread Kevin Hilman 
On Wed, Nov 23, 2016 at 9:03 PM, Sekhar Nori  wrote:
> On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
>> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>>> David Lechner  writes:
>>>
 On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
> 2016-11-22 23:23 GMT+01:00 David Lechner :
>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>>
>>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>>> controller drivers to da850.dtsi.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>> v1 -> v2:
>>> - moved the priority controller node above the cfgchip node
>>> - renamed added nodes to better reflect their purpose
>>>
>>>  arch/arm/boot/dts/da850.dtsi | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>> b/arch/arm/boot/dts/da850.dtsi
>>> index 1bb1f6d..412eec6 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -210,6 +210,10 @@
>>> };
>>>
>>> };
>>> +   prictrl: priority-controller@14110 {
>>> +   compatible = "ti,da850-mstpri";
>>> +   reg = <0x14110 0x0c>;
>>
>>
>> I think we should add status = "disabled"; here and let boards opt in.
>>
>>> +   };
>>> cfgchip: chip-controller@1417c {
>>> compatible = "ti,da830-cfgchip", "syscon",
>>> "simple-mfd";
>>> reg = <0x1417c 0x14>;
>>> @@ -451,4 +455,8 @@
>>>   1 0 0x6800 0x8000>;
>>> status = "disabled";
>>> };
>>> +   memctrl: memory-controller@b000 {
>>> +   compatible = "ti,da850-ddr-controller";
>>> +   reg = <0xb000 0xe8>;
>>
>>
>> same here. status = "disabled";
>>
>>> +   };
>>>  };
>>>
>
> Hi David,
>
> I did that initially[1][2] and it was rejected by Kevin[3] and
> Laurent[4].
>
> FYI this patch has already been queued by Sekhar.

 Thanks. I did not see those threads.

 FYI to maintainers, having these enabled by default causes error
 messages in the kernel log for other boards that are not supported by
 the drivers.
>>>
>>> Then the driver is too noisy and should be cleaned up.
>>>
 Since there is only one board that is supported and soon
 to be 2 that are not, I would rather have this disabled by default to
 avoid the error messages.
>>>
>>> IMO, what exactly are the error messages? Sounds like the driver is
>>> being too verbose, and calling things errors that are not really errors.
>>
>> It is just one line per driver.
>>
>> dev_err(dev, "no master priorities defined for this board\n");
>>
>> and
>>
>> dev_err(dev, "no settings defined for this board\n");
>>
>>
>> Since "ti,da850-lcdk" is the only board supported in these drivers, all
>> other boards will see these error messages.
>
> Thats pretty bad. Sorry about that. The original justification for
> keeping them enabled all the time was that they are in-SoC modules with
> no external dependencies (like IO lines or voltage rails) so they can be
> enabled on all boards that use DA850. While that remains true, the
> configuration itself is board specific.
>
> I think the error messages are still useful, so instead of silencing
> them, I think we should go back to keeping these nodes disabled by
> default and enabling only on boards which have support for it in the driver.

I don't have a strong preference for the enabled/disabled by default,
but I think the error messages are not error messages.  It seems
perfectly reasonable for boards to accept the reset (or bootloader)
configuration of these registers, and not consider that an error.

Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-27 Thread Kevin Hilman 
On Wed, Nov 23, 2016 at 9:03 PM, Sekhar Nori  wrote:
> On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
>> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>>> David Lechner  writes:
>>>
 On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
> 2016-11-22 23:23 GMT+01:00 David Lechner :
>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>>
>>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>>> controller drivers to da850.dtsi.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>> v1 -> v2:
>>> - moved the priority controller node above the cfgchip node
>>> - renamed added nodes to better reflect their purpose
>>>
>>>  arch/arm/boot/dts/da850.dtsi | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>> b/arch/arm/boot/dts/da850.dtsi
>>> index 1bb1f6d..412eec6 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -210,6 +210,10 @@
>>> };
>>>
>>> };
>>> +   prictrl: priority-controller@14110 {
>>> +   compatible = "ti,da850-mstpri";
>>> +   reg = <0x14110 0x0c>;
>>
>>
>> I think we should add status = "disabled"; here and let boards opt in.
>>
>>> +   };
>>> cfgchip: chip-controller@1417c {
>>> compatible = "ti,da830-cfgchip", "syscon",
>>> "simple-mfd";
>>> reg = <0x1417c 0x14>;
>>> @@ -451,4 +455,8 @@
>>>   1 0 0x6800 0x8000>;
>>> status = "disabled";
>>> };
>>> +   memctrl: memory-controller@b000 {
>>> +   compatible = "ti,da850-ddr-controller";
>>> +   reg = <0xb000 0xe8>;
>>
>>
>> same here. status = "disabled";
>>
>>> +   };
>>>  };
>>>
>
> Hi David,
>
> I did that initially[1][2] and it was rejected by Kevin[3] and
> Laurent[4].
>
> FYI this patch has already been queued by Sekhar.

 Thanks. I did not see those threads.

 FYI to maintainers, having these enabled by default causes error
 messages in the kernel log for other boards that are not supported by
 the drivers.
>>>
>>> Then the driver is too noisy and should be cleaned up.
>>>
 Since there is only one board that is supported and soon
 to be 2 that are not, I would rather have this disabled by default to
 avoid the error messages.
>>>
>>> IMO, what exactly are the error messages? Sounds like the driver is
>>> being too verbose, and calling things errors that are not really errors.
>>
>> It is just one line per driver.
>>
>> dev_err(dev, "no master priorities defined for this board\n");
>>
>> and
>>
>> dev_err(dev, "no settings defined for this board\n");
>>
>>
>> Since "ti,da850-lcdk" is the only board supported in these drivers, all
>> other boards will see these error messages.
>
> Thats pretty bad. Sorry about that. The original justification for
> keeping them enabled all the time was that they are in-SoC modules with
> no external dependencies (like IO lines or voltage rails) so they can be
> enabled on all boards that use DA850. While that remains true, the
> configuration itself is board specific.
>
> I think the error messages are still useful, so instead of silencing
> them, I think we should go back to keeping these nodes disabled by
> default and enabling only on boards which have support for it in the driver.

I don't have a strong preference for the enabled/disabled by default,
but I think the error messages are not error messages.  It seems
perfectly reasonable for boards to accept the reset (or bootloader)
configuration of these registers, and not consider that an error.

Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-24 Thread Bartosz Golaszewski 
2016-11-24 6:03 GMT+01:00 Sekhar Nori :
> On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
>> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>>> David Lechner  writes:
>>>
 On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
> 2016-11-22 23:23 GMT+01:00 David Lechner :
>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>>
>>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>>> controller drivers to da850.dtsi.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>> v1 -> v2:
>>> - moved the priority controller node above the cfgchip node
>>> - renamed added nodes to better reflect their purpose
>>>
>>>  arch/arm/boot/dts/da850.dtsi | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>> b/arch/arm/boot/dts/da850.dtsi
>>> index 1bb1f6d..412eec6 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -210,6 +210,10 @@
>>> };
>>>
>>> };
>>> +   prictrl: priority-controller@14110 {
>>> +   compatible = "ti,da850-mstpri";
>>> +   reg = <0x14110 0x0c>;
>>
>>
>> I think we should add status = "disabled"; here and let boards opt in.
>>
>>> +   };
>>> cfgchip: chip-controller@1417c {
>>> compatible = "ti,da830-cfgchip", "syscon",
>>> "simple-mfd";
>>> reg = <0x1417c 0x14>;
>>> @@ -451,4 +455,8 @@
>>>   1 0 0x6800 0x8000>;
>>> status = "disabled";
>>> };
>>> +   memctrl: memory-controller@b000 {
>>> +   compatible = "ti,da850-ddr-controller";
>>> +   reg = <0xb000 0xe8>;
>>
>>
>> same here. status = "disabled";
>>
>>> +   };
>>>  };
>>>
>
> Hi David,
>
> I did that initially[1][2] and it was rejected by Kevin[3] and
> Laurent[4].
>
> FYI this patch has already been queued by Sekhar.

 Thanks. I did not see those threads.

 FYI to maintainers, having these enabled by default causes error
 messages in the kernel log for other boards that are not supported by
 the drivers.
>>>
>>> Then the driver is too noisy and should be cleaned up.
>>>
 Since there is only one board that is supported and soon
 to be 2 that are not, I would rather have this disabled by default to
 avoid the error messages.
>>>
>>> IMO, what exactly are the error messages? Sounds like the driver is
>>> being too verbose, and calling things errors that are not really errors.
>>
>> It is just one line per driver.
>>
>> dev_err(dev, "no master priorities defined for this board\n");
>>
>> and
>>
>> dev_err(dev, "no settings defined for this board\n");
>>
>>
>> Since "ti,da850-lcdk" is the only board supported in these drivers, all
>> other boards will see these error messages.
>
> Thats pretty bad. Sorry about that. The original justification for
> keeping them enabled all the time was that they are in-SoC modules with
> no external dependencies (like IO lines or voltage rails) so they can be
> enabled on all boards that use DA850. While that remains true, the
> configuration itself is board specific.
>
> I think the error messages are still useful, so instead of silencing
> them, I think we should go back to keeping these nodes disabled by
> default and enabling only on boards which have support for it in the driver.
>
> Thanks,
> Sekhar

I'll send a patch.

Thanks,
Bartosz

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-24 Thread Bartosz Golaszewski 
2016-11-24 6:03 GMT+01:00 Sekhar Nori :
> On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
>> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>>> David Lechner  writes:
>>>
 On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
> 2016-11-22 23:23 GMT+01:00 David Lechner :
>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>>
>>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>>> controller drivers to da850.dtsi.
>>>
>>> Signed-off-by: Bartosz Golaszewski 
>>> ---
>>> v1 -> v2:
>>> - moved the priority controller node above the cfgchip node
>>> - renamed added nodes to better reflect their purpose
>>>
>>>  arch/arm/boot/dts/da850.dtsi | 8 
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi
>>> b/arch/arm/boot/dts/da850.dtsi
>>> index 1bb1f6d..412eec6 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -210,6 +210,10 @@
>>> };
>>>
>>> };
>>> +   prictrl: priority-controller@14110 {
>>> +   compatible = "ti,da850-mstpri";
>>> +   reg = <0x14110 0x0c>;
>>
>>
>> I think we should add status = "disabled"; here and let boards opt in.
>>
>>> +   };
>>> cfgchip: chip-controller@1417c {
>>> compatible = "ti,da830-cfgchip", "syscon",
>>> "simple-mfd";
>>> reg = <0x1417c 0x14>;
>>> @@ -451,4 +455,8 @@
>>>   1 0 0x6800 0x8000>;
>>> status = "disabled";
>>> };
>>> +   memctrl: memory-controller@b000 {
>>> +   compatible = "ti,da850-ddr-controller";
>>> +   reg = <0xb000 0xe8>;
>>
>>
>> same here. status = "disabled";
>>
>>> +   };
>>>  };
>>>
>
> Hi David,
>
> I did that initially[1][2] and it was rejected by Kevin[3] and
> Laurent[4].
>
> FYI this patch has already been queued by Sekhar.

 Thanks. I did not see those threads.

 FYI to maintainers, having these enabled by default causes error
 messages in the kernel log for other boards that are not supported by
 the drivers.
>>>
>>> Then the driver is too noisy and should be cleaned up.
>>>
 Since there is only one board that is supported and soon
 to be 2 that are not, I would rather have this disabled by default to
 avoid the error messages.
>>>
>>> IMO, what exactly are the error messages? Sounds like the driver is
>>> being too verbose, and calling things errors that are not really errors.
>>
>> It is just one line per driver.
>>
>> dev_err(dev, "no master priorities defined for this board\n");
>>
>> and
>>
>> dev_err(dev, "no settings defined for this board\n");
>>
>>
>> Since "ti,da850-lcdk" is the only board supported in these drivers, all
>> other boards will see these error messages.
>
> Thats pretty bad. Sorry about that. The original justification for
> keeping them enabled all the time was that they are in-SoC modules with
> no external dependencies (like IO lines or voltage rails) so they can be
> enabled on all boards that use DA850. While that remains true, the
> configuration itself is board specific.
>
> I think the error messages are still useful, so instead of silencing
> them, I think we should go back to keeping these nodes disabled by
> default and enabling only on boards which have support for it in the driver.
>
> Thanks,
> Sekhar

I'll send a patch.

Thanks,
Bartosz

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Sekhar Nori 
On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>> David Lechner  writes:
>>
>>> On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
 2016-11-22 23:23 GMT+01:00 David Lechner :
> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>
>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>> controller drivers to da850.dtsi.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>> v1 -> v2:
>> - moved the priority controller node above the cfgchip node
>> - renamed added nodes to better reflect their purpose
>>
>>  arch/arm/boot/dts/da850.dtsi | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi
>> b/arch/arm/boot/dts/da850.dtsi
>> index 1bb1f6d..412eec6 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -210,6 +210,10 @@
>> };
>>
>> };
>> +   prictrl: priority-controller@14110 {
>> +   compatible = "ti,da850-mstpri";
>> +   reg = <0x14110 0x0c>;
>
>
> I think we should add status = "disabled"; here and let boards opt in.
>
>> +   };
>> cfgchip: chip-controller@1417c {
>> compatible = "ti,da830-cfgchip", "syscon",
>> "simple-mfd";
>> reg = <0x1417c 0x14>;
>> @@ -451,4 +455,8 @@
>>   1 0 0x6800 0x8000>;
>> status = "disabled";
>> };
>> +   memctrl: memory-controller@b000 {
>> +   compatible = "ti,da850-ddr-controller";
>> +   reg = <0xb000 0xe8>;
>
>
> same here. status = "disabled";
>
>> +   };
>>  };
>>

 Hi David,

 I did that initially[1][2] and it was rejected by Kevin[3] and
 Laurent[4].

 FYI this patch has already been queued by Sekhar.
>>>
>>> Thanks. I did not see those threads.
>>>
>>> FYI to maintainers, having these enabled by default causes error
>>> messages in the kernel log for other boards that are not supported by
>>> the drivers.
>>
>> Then the driver is too noisy and should be cleaned up.
>>
>>> Since there is only one board that is supported and soon
>>> to be 2 that are not, I would rather have this disabled by default to
>>> avoid the error messages.
>>
>> IMO, what exactly are the error messages? Sounds like the driver is
>> being too verbose, and calling things errors that are not really errors.
> 
> It is just one line per driver.
> 
> dev_err(dev, "no master priorities defined for this board\n");
> 
> and
> 
> dev_err(dev, "no settings defined for this board\n");
> 
> 
> Since "ti,da850-lcdk" is the only board supported in these drivers, all
> other boards will see these error messages.

Thats pretty bad. Sorry about that. The original justification for
keeping them enabled all the time was that they are in-SoC modules with
no external dependencies (like IO lines or voltage rails) so they can be
enabled on all boards that use DA850. While that remains true, the
configuration itself is board specific.

I think the error messages are still useful, so instead of silencing
them, I think we should go back to keeping these nodes disabled by
default and enabling only on boards which have support for it in the driver.

Thanks,
Sekhar

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Sekhar Nori 
On Thursday 24 November 2016 04:18 AM, David Lechner wrote:
> On 11/23/2016 04:32 PM, Kevin Hilman wrote:
>> David Lechner  writes:
>>
>>> On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
 2016-11-22 23:23 GMT+01:00 David Lechner :
> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>
>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>> controller drivers to da850.dtsi.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>> v1 -> v2:
>> - moved the priority controller node above the cfgchip node
>> - renamed added nodes to better reflect their purpose
>>
>>  arch/arm/boot/dts/da850.dtsi | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi
>> b/arch/arm/boot/dts/da850.dtsi
>> index 1bb1f6d..412eec6 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -210,6 +210,10 @@
>> };
>>
>> };
>> +   prictrl: priority-controller@14110 {
>> +   compatible = "ti,da850-mstpri";
>> +   reg = <0x14110 0x0c>;
>
>
> I think we should add status = "disabled"; here and let boards opt in.
>
>> +   };
>> cfgchip: chip-controller@1417c {
>> compatible = "ti,da830-cfgchip", "syscon",
>> "simple-mfd";
>> reg = <0x1417c 0x14>;
>> @@ -451,4 +455,8 @@
>>   1 0 0x6800 0x8000>;
>> status = "disabled";
>> };
>> +   memctrl: memory-controller@b000 {
>> +   compatible = "ti,da850-ddr-controller";
>> +   reg = <0xb000 0xe8>;
>
>
> same here. status = "disabled";
>
>> +   };
>>  };
>>

 Hi David,

 I did that initially[1][2] and it was rejected by Kevin[3] and
 Laurent[4].

 FYI this patch has already been queued by Sekhar.
>>>
>>> Thanks. I did not see those threads.
>>>
>>> FYI to maintainers, having these enabled by default causes error
>>> messages in the kernel log for other boards that are not supported by
>>> the drivers.
>>
>> Then the driver is too noisy and should be cleaned up.
>>
>>> Since there is only one board that is supported and soon
>>> to be 2 that are not, I would rather have this disabled by default to
>>> avoid the error messages.
>>
>> IMO, what exactly are the error messages? Sounds like the driver is
>> being too verbose, and calling things errors that are not really errors.
> 
> It is just one line per driver.
> 
> dev_err(dev, "no master priorities defined for this board\n");
> 
> and
> 
> dev_err(dev, "no settings defined for this board\n");
> 
> 
> Since "ti,da850-lcdk" is the only board supported in these drivers, all
> other boards will see these error messages.

Thats pretty bad. Sorry about that. The original justification for
keeping them enabled all the time was that they are in-SoC modules with
no external dependencies (like IO lines or voltage rails) so they can be
enabled on all boards that use DA850. While that remains true, the
configuration itself is board specific.

I think the error messages are still useful, so instead of silencing
them, I think we should go back to keeping these nodes disabled by
default and enabling only on boards which have support for it in the driver.

Thanks,
Sekhar

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread David Lechner 

On 11/23/2016 04:32 PM, Kevin Hilman wrote:

David Lechner  writes:


On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:

2016-11-22 23:23 GMT+01:00 David Lechner :

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:


Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;



I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon",
"simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;



same here. status = "disabled";


+   };
 };



Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.


Thanks. I did not see those threads.

FYI to maintainers, having these enabled by default causes error
messages in the kernel log for other boards that are not supported by
the drivers.


Then the driver is too noisy and should be cleaned up.


Since there is only one board that is supported and soon
to be 2 that are not, I would rather have this disabled by default to
avoid the error messages.


IMO, what exactly are the error messages? Sounds like the driver is
being too verbose, and calling things errors that are not really errors.


It is just one line per driver.

dev_err(dev, "no master priorities defined for this board\n");

and

dev_err(dev, "no settings defined for this board\n");


Since "ti,da850-lcdk" is the only board supported in these drivers, all 
other boards will see these error messages.


Also, these modules will be loaded and taking up memory on boards that 
don't use them. This not really a big deal and they can be explicitly 
disabled, so maybe it was not worth mentioning.





Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread David Lechner 

On 11/23/2016 04:32 PM, Kevin Hilman wrote:

David Lechner  writes:


On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:

2016-11-22 23:23 GMT+01:00 David Lechner :

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:


Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;



I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon",
"simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;



same here. status = "disabled";


+   };
 };



Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.


Thanks. I did not see those threads.

FYI to maintainers, having these enabled by default causes error
messages in the kernel log for other boards that are not supported by
the drivers.


Then the driver is too noisy and should be cleaned up.


Since there is only one board that is supported and soon
to be 2 that are not, I would rather have this disabled by default to
avoid the error messages.


IMO, what exactly are the error messages? Sounds like the driver is
being too verbose, and calling things errors that are not really errors.


It is just one line per driver.

dev_err(dev, "no master priorities defined for this board\n");

and

dev_err(dev, "no settings defined for this board\n");


Since "ti,da850-lcdk" is the only board supported in these drivers, all 
other boards will see these error messages.


Also, these modules will be loaded and taking up memory on boards that 
don't use them. This not really a big deal and they can be explicitly 
disabled, so maybe it was not worth mentioning.





Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Kevin Hilman 
David Lechner  writes:

> On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
>> 2016-11-22 23:23 GMT+01:00 David Lechner :
>>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:

 Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
 controller drivers to da850.dtsi.

 Signed-off-by: Bartosz Golaszewski 
 ---
 v1 -> v2:
 - moved the priority controller node above the cfgchip node
 - renamed added nodes to better reflect their purpose

  arch/arm/boot/dts/da850.dtsi | 8 
  1 file changed, 8 insertions(+)

 diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
 index 1bb1f6d..412eec6 100644
 --- a/arch/arm/boot/dts/da850.dtsi
 +++ b/arch/arm/boot/dts/da850.dtsi
 @@ -210,6 +210,10 @@
 };

 };
 +   prictrl: priority-controller@14110 {
 +   compatible = "ti,da850-mstpri";
 +   reg = <0x14110 0x0c>;
>>>
>>>
>>> I think we should add status = "disabled"; here and let boards opt in.
>>>
 +   };
 cfgchip: chip-controller@1417c {
 compatible = "ti,da830-cfgchip", "syscon",
 "simple-mfd";
 reg = <0x1417c 0x14>;
 @@ -451,4 +455,8 @@
   1 0 0x6800 0x8000>;
 status = "disabled";
 };
 +   memctrl: memory-controller@b000 {
 +   compatible = "ti,da850-ddr-controller";
 +   reg = <0xb000 0xe8>;
>>>
>>>
>>> same here. status = "disabled";
>>>
 +   };
  };

>>
>> Hi David,
>>
>> I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].
>>
>> FYI this patch has already been queued by Sekhar.
>
> Thanks. I did not see those threads.
>
> FYI to maintainers, having these enabled by default causes error
> messages in the kernel log for other boards that are not supported by
> the drivers.

Then the driver is too noisy and should be cleaned up.

> Since there is only one board that is supported and soon
> to be 2 that are not, I would rather have this disabled by default to
> avoid the error messages.

IMO, what exactly are the error messages? Sounds like the driver is
being too verbose, and calling things errors that are not really errors.

Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Kevin Hilman 
David Lechner  writes:

> On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:
>> 2016-11-22 23:23 GMT+01:00 David Lechner :
>>> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:

 Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
 controller drivers to da850.dtsi.

 Signed-off-by: Bartosz Golaszewski 
 ---
 v1 -> v2:
 - moved the priority controller node above the cfgchip node
 - renamed added nodes to better reflect their purpose

  arch/arm/boot/dts/da850.dtsi | 8 
  1 file changed, 8 insertions(+)

 diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
 index 1bb1f6d..412eec6 100644
 --- a/arch/arm/boot/dts/da850.dtsi
 +++ b/arch/arm/boot/dts/da850.dtsi
 @@ -210,6 +210,10 @@
 };

 };
 +   prictrl: priority-controller@14110 {
 +   compatible = "ti,da850-mstpri";
 +   reg = <0x14110 0x0c>;
>>>
>>>
>>> I think we should add status = "disabled"; here and let boards opt in.
>>>
 +   };
 cfgchip: chip-controller@1417c {
 compatible = "ti,da830-cfgchip", "syscon",
 "simple-mfd";
 reg = <0x1417c 0x14>;
 @@ -451,4 +455,8 @@
   1 0 0x6800 0x8000>;
 status = "disabled";
 };
 +   memctrl: memory-controller@b000 {
 +   compatible = "ti,da850-ddr-controller";
 +   reg = <0xb000 0xe8>;
>>>
>>>
>>> same here. status = "disabled";
>>>
 +   };
  };

>>
>> Hi David,
>>
>> I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].
>>
>> FYI this patch has already been queued by Sekhar.
>
> Thanks. I did not see those threads.
>
> FYI to maintainers, having these enabled by default causes error
> messages in the kernel log for other boards that are not supported by
> the drivers.

Then the driver is too noisy and should be cleaned up.

> Since there is only one board that is supported and soon
> to be 2 that are not, I would rather have this disabled by default to
> avoid the error messages.

IMO, what exactly are the error messages? Sounds like the driver is
being too verbose, and calling things errors that are not really errors.

Kevin

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread David Lechner 

On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:

2016-11-22 23:23 GMT+01:00 David Lechner :

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:


Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;



I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon",
"simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;



same here. status = "disabled";


+   };
 };



Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.


Thanks. I did not see those threads.

FYI to maintainers, having these enabled by default causes error 
messages in the kernel log for other boards that are not supported by 
the drivers. Since there is only one board that is supported and soon to 
be 2 that are not, I would rather have this disabled by default to avoid 
the error messages.




Best regards,
Bartosz Golaszewski

[1] https://www.spinics.net/lists/arm-kernel/msg539638.html
[2] http://www.spinics.net/lists/devicetree/msg148575.html
[3] http://www.spinics.net/lists/devicetree/msg148667.html
[4] http://www.spinics.net/lists/devicetree/msg148655.html

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread David Lechner 

On 11/23/2016 04:27 AM, Bartosz Golaszewski wrote:

2016-11-22 23:23 GMT+01:00 David Lechner :

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:


Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;



I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon",
"simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;



same here. status = "disabled";


+   };
 };



Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.


Thanks. I did not see those threads.

FYI to maintainers, having these enabled by default causes error 
messages in the kernel log for other boards that are not supported by 
the drivers. Since there is only one board that is supported and soon to 
be 2 that are not, I would rather have this disabled by default to avoid 
the error messages.




Best regards,
Bartosz Golaszewski

[1] https://www.spinics.net/lists/arm-kernel/msg539638.html
[2] http://www.spinics.net/lists/devicetree/msg148575.html
[3] http://www.spinics.net/lists/devicetree/msg148667.html
[4] http://www.spinics.net/lists/devicetree/msg148655.html

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Bartosz Golaszewski 
2016-11-22 23:23 GMT+01:00 David Lechner :
> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>
>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>> controller drivers to da850.dtsi.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>> v1 -> v2:
>> - moved the priority controller node above the cfgchip node
>> - renamed added nodes to better reflect their purpose
>>
>>  arch/arm/boot/dts/da850.dtsi | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 1bb1f6d..412eec6 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -210,6 +210,10 @@
>> };
>>
>> };
>> +   prictrl: priority-controller@14110 {
>> +   compatible = "ti,da850-mstpri";
>> +   reg = <0x14110 0x0c>;
>
>
> I think we should add status = "disabled"; here and let boards opt in.
>
>> +   };
>> cfgchip: chip-controller@1417c {
>> compatible = "ti,da830-cfgchip", "syscon",
>> "simple-mfd";
>> reg = <0x1417c 0x14>;
>> @@ -451,4 +455,8 @@
>>   1 0 0x6800 0x8000>;
>> status = "disabled";
>> };
>> +   memctrl: memory-controller@b000 {
>> +   compatible = "ti,da850-ddr-controller";
>> +   reg = <0xb000 0xe8>;
>
>
> same here. status = "disabled";
>
>> +   };
>>  };
>>

Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.

Best regards,
Bartosz Golaszewski

[1] https://www.spinics.net/lists/arm-kernel/msg539638.html
[2] http://www.spinics.net/lists/devicetree/msg148575.html
[3] http://www.spinics.net/lists/devicetree/msg148667.html
[4] http://www.spinics.net/lists/devicetree/msg148655.html

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-23 Thread Bartosz Golaszewski 
2016-11-22 23:23 GMT+01:00 David Lechner :
> On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:
>>
>> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
>> controller drivers to da850.dtsi.
>>
>> Signed-off-by: Bartosz Golaszewski 
>> ---
>> v1 -> v2:
>> - moved the priority controller node above the cfgchip node
>> - renamed added nodes to better reflect their purpose
>>
>>  arch/arm/boot/dts/da850.dtsi | 8 
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index 1bb1f6d..412eec6 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -210,6 +210,10 @@
>> };
>>
>> };
>> +   prictrl: priority-controller@14110 {
>> +   compatible = "ti,da850-mstpri";
>> +   reg = <0x14110 0x0c>;
>
>
> I think we should add status = "disabled"; here and let boards opt in.
>
>> +   };
>> cfgchip: chip-controller@1417c {
>> compatible = "ti,da830-cfgchip", "syscon",
>> "simple-mfd";
>> reg = <0x1417c 0x14>;
>> @@ -451,4 +455,8 @@
>>   1 0 0x6800 0x8000>;
>> status = "disabled";
>> };
>> +   memctrl: memory-controller@b000 {
>> +   compatible = "ti,da850-ddr-controller";
>> +   reg = <0xb000 0xe8>;
>
>
> same here. status = "disabled";
>
>> +   };
>>  };
>>

Hi David,

I did that initially[1][2] and it was rejected by Kevin[3] and Laurent[4].

FYI this patch has already been queued by Sekhar.

Best regards,
Bartosz Golaszewski

[1] https://www.spinics.net/lists/arm-kernel/msg539638.html
[2] http://www.spinics.net/lists/devicetree/msg148575.html
[3] http://www.spinics.net/lists/devicetree/msg148667.html
[4] http://www.spinics.net/lists/devicetree/msg148655.html

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-22 Thread David Lechner 

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:

Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;


I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;


same here. status = "disabled";


+   };
 };

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-22 Thread David Lechner 

On 11/15/2016 05:00 AM, Bartosz Golaszewski wrote:

Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};

};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;


I think we should add status = "disabled"; here and let boards opt in.


+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;


same here. status = "disabled";


+   };
 };

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-16 Thread Sekhar Nori 
On Tuesday 15 November 2016 04:30 PM, Bartosz Golaszewski wrote:
> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
> controller drivers to da850.dtsi.
> 
> Signed-off-by: Bartosz Golaszewski 

Applied to v4.10/dt

Thanks,
Sekhar

Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-16 Thread Sekhar Nori 
On Tuesday 15 November 2016 04:30 PM, Bartosz Golaszewski wrote:
> Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
> controller drivers to da850.dtsi.
> 
> Signed-off-by: Bartosz Golaszewski 

Applied to v4.10/dt

Thanks,
Sekhar

[PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-15 Thread Bartosz Golaszewski 
Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};
 
};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;
+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;
+   };
 };
-- 
2.9.3

[PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes

2016-11-15 Thread Bartosz Golaszewski 
Add the nodes for the MSTPRI configuration and DDR2/mDDR memory
controller drivers to da850.dtsi.

Signed-off-by: Bartosz Golaszewski 
---
v1 -> v2:
- moved the priority controller node above the cfgchip node
- renamed added nodes to better reflect their purpose

 arch/arm/boot/dts/da850.dtsi | 8 
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 1bb1f6d..412eec6 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -210,6 +210,10 @@
};
 
};
+   prictrl: priority-controller@14110 {
+   compatible = "ti,da850-mstpri";
+   reg = <0x14110 0x0c>;
+   };
cfgchip: chip-controller@1417c {
compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
reg = <0x1417c 0x14>;
@@ -451,4 +455,8 @@
  1 0 0x6800 0x8000>;
status = "disabled";
};
+   memctrl: memory-controller@b000 {
+   compatible = "ti,da850-ddr-controller";
+   reg = <0xb000 0xe8>;
+   };
 };
-- 
2.9.3