Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756991AbcKXIsq (ORCPT ); Thu, 24 Nov 2016 03:48:46 -0500 Received: from mail-vk0-f50.google.com ([209.85.213.50]:35582 "EHLO mail-vk0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756892AbcKXIsn (ORCPT ); Thu, 24 Nov 2016 03:48:43 -0500 MIME-Version: 1.0 In-Reply-To: <20fbc946-d56c-31a3-4ae7-cf61df96a3c3@ti.com> References: <1479207611-18028-1-git-send-email-bgolaszewski@baylibre.com> <5e647eb0-2f8a-b46b-2048-7616bfb54ad7@lechnology.com> <20fbc946-d56c-31a3-4ae7-cf61df96a3c3@ti.com> From: Bartosz Golaszewski Date: Thu, 24 Nov 2016 09:48:41 +0100 Message-ID: Subject: Re: [PATCH v2] ARM: dts: da850: add the mstpri and ddrctl nodes To: Sekhar Nori Cc: David Lechner , Kevin Hilman , Michael Turquette , Rob Herring , Frank Rowand , Mark Rutland , Peter Ujfalusi , Russell King , linux-devicetree , David Airlie , LKML , linux-drm , Tomi Valkeinen , Jyri Sarha , arm-soc , Laurent Pinchart Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3820 Lines: 108 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 0x68000000 0x00008000>; >>>>>>> status = "disabled"; >>>>>>> }; >>>>>>> + memctrl: memory-controller@b0000000 { >>>>>>> + compatible = "ti,da850-ddr-controller"; >>>>>>> + reg = <0xb0000000 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