linux-kernel - Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56B1C19F.9070300@gmail.com>
Date:	Wed, 3 Feb 2016 10:00:15 +0100
From:	Matthias Brugger <matthias.bgg@...il.com>
To:	James Liao <jamesjj.liao@...iatek.com>
Cc:	Sascha Hauer <kernel@...gutronix.de>,
	Rob Herring <robh@...nel.org>,
	Kevin Hilman <khilman@...nel.org>,
	Daniel Kurtz <djkurtz@...omium.org>,
	srv_heupstream@...iatek.com, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH v4 1/4] soc: mediatek: Refine scpsys to support multiple
 platform



On 03/02/16 06:22, James Liao wrote:
> Hi Matthias,
>
> On Tue, 2016-02-02 at 11:44 +0100, Matthias Brugger wrote:
>> On 02/02/16 07:56, James Liao wrote:
>>> On Sun, 2016-01-31 at 12:51 +0100, Matthias Brugger wrote:
>>>>> On 20/01/16 07:08, James Liao wrote:
>>>>>>> Refine scpsys driver common code to support multiple SoC / platform.
>>>>>>>
>>>>>>> Signed-off-by: James Liao<jamesjj.liao@...iatek.com>
>>>>>>> ---
>>>>>>>    drivers/soc/mediatek/mtk-scpsys.c | 418 ++++++++++++++++++++------------------
>>>>>>>    drivers/soc/mediatek/mtk-scpsys.h |  55 +++++
>>>>>>>    2 files changed, 270 insertions(+), 203 deletions(-)
>>>>>>>    create mode 100644 drivers/soc/mediatek/mtk-scpsys.h
>>>>>
>>>>> In general this approach looks fine to me, comments below.
>>>>>
>>>>>>>
>>>>>>> diff --git a/drivers/soc/mediatek/mtk-scpsys.c b/drivers/soc/mediatek/mtk-scpsys.c
>>>>>>> index 0221387..339adfc 100644
>>>>>>> --- a/drivers/soc/mediatek/mtk-scpsys.c
>>>>>>> +++ b/drivers/soc/mediatek/mtk-scpsys.c
>>>>>>> @@ -11,29 +11,17 @@
>>>>>>>     * GNU General Public License for more details.
>>>>>>>     */
>>>>>>>    #include <linux/clk.h>
>>>>>>> -#include <linux/delay.h>
>>>>>>> +#include <linux/init.h>
>>>>>>>    #include <linux/io.h>
>>>>>>> -#include <linux/kernel.h>
>>>>>>>    #include <linux/mfd/syscon.h>
>>>>>
>>>>> When at it, do we need this include?
>>> syscon_regmap_lookup_by_phandle() is declared in this head file.
>>>
>>>>>>> -#include <linux/init.h>
>>>>>>>    #include <linux/of_device.h>
>>>>>>>    #include <linux/platform_device.h>
>>>>>>>    #include <linux/pm_domain.h>
>>>>>>> -#include <linux/regmap.h>
>>>>>>> -#include <linux/soc/mediatek/infracfg.h>
>>>>>>>    #include <linux/regulator/consumer.h>
>>>>>>> -#include <dt-bindings/power/mt8173-power.h>
>>>>>>> +#include <linux/soc/mediatek/infracfg.h>
>>>>>>> +
>>>>>>> +#include "mtk-scpsys.h"
>>>>>>>
>>>>>>> -#define SPM_VDE_PWR_CON			0x0210
>>>>>>> -#define SPM_MFG_PWR_CON			0x0214
>>>>>>> -#define SPM_VEN_PWR_CON			0x0230
>>>>>>> -#define SPM_ISP_PWR_CON			0x0238
>>>>>>> -#define SPM_DIS_PWR_CON			0x023c
>>>>>>> -#define SPM_VEN2_PWR_CON		0x0298
>>>>>>> -#define SPM_AUDIO_PWR_CON		0x029c
>>>>>>> -#define SPM_MFG_2D_PWR_CON		0x02c0
>>>>>>> -#define SPM_MFG_ASYNC_PWR_CON		0x02c4
>>>>>>> -#define SPM_USB_PWR_CON			0x02cc
>>>>>
>>>>> I would prefer to keep this defines and declare SoC specific ones where
>>>>> necessary. It makes the code more readable.
>>> Some register address may be reused by other modules among SoCs, so it's
>>> not easy to maintain the defines when we implement multiple SoC drivers
>>> in the same file. For example, offset 0x0298 is VEN2_PWR_CON on MT8173,
>>> but it is MJC_PWR_CON on other chips.
>>>
>>
>> So that sounds as if 0x0298 offset is MT8173 specific.
>> I checked [VDE, MFG, VEN, IFR, ISP, DIS, DPY]_PWR_CON on mt8173, mt8135
>> and mt6589 and they all have the same offset. So it doesn't seem as if
>> the offset randomly changes for every SoC.
>>
>>> Furthermore, these register offsets are only used in scp_domain_data[],
>>> and each element has its own power domain name. So I think it's enough
>>> to know which power domain are using these registers and status bits.
>>>
>>
>> Yes that's true, but it will make it easier for another person to
>> understand the driver, especially if he want's to implement the driver
>> for a new SoC.
>
> There are two kinds of conflicts may happen:
>
> 1. Different modules use the same register address.
> 2. Different register addresses are used by the same module (on
> different IC).
>
> Type 1. for example:
>
> 	#define SPM_BDP_PWR_CON			0x029c /* 2701 */
> 	#define SPM_AUDIO_PWR_CON		0x029c /* 8173 */
>
> We can resolve this conflict easily, such as define these two register
> name to the same register address.
>
> Type 2. for example:
>
> 	#define SPM_VDE_PWR_CON			0x0300 /* 6755 */
> 	#define SPM_VDE_PWR_CON			0x0210 /* 8173 */
>
> We can not reuse the register defines in this case. We may need to name
> the registers with its IC name, such as MT8173_SPM_VDE_PWR_CON and
> MT6755_VDE_PWR_CON. But it will increase the maintain effort. That's why
> I prefer to remove register defines if we implement multiple SoC's
> scpsys in a single file.
>
>

Well type 2 for me is no problem at all. As stated in my last mail, 
mt6755 would get the SoC name in the define (as a postfix preferably).
I don't think that this will make a lot of pain regarding maintaining 
it. Even less if we have the defines in alphabetic order.

I we see in the future that this converts to a mess, we always can get 
rid of the defines quite easily.

Regards,
Matthias