Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx

From: Suman Anna <s-anna@ti.com>
To: Tero Kristo <t-kristo@ti.com>, <ssantosh@kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-omap@vger.kernel.org>, <robh+dt@kernel.org>
Cc: tony@atomide.com, devicetree@vger.kernel.org
Subject: Re: [PATCH 6/8] soc: ti: omap_prm: add data for am33xx
Date: Wed, 21 Aug 2019 10:49:24 -0500	[thread overview]
Message-ID: <ca2c21c9-ddcd-e378-ca2b-435e91c87700@ti.com> (raw)
In-Reply-To: <8f5f86db-270a-7278-9d9c-e84c0fa9b73c@ti.com>

On 8/21/19 2:23 AM, Tero Kristo wrote:
> On 20.8.2019 21.48, Suman Anna wrote:
>> Hi Tero,
>>
>> On 8/7/19 2:48 AM, Tero Kristo wrote:
>>> Add PRM instance data for AM33xx SoC. Includes some basic register
>>> definitions and reset data for now.
>>>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> ---
>>>   drivers/soc/ti/omap_prm.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/soc/ti/omap_prm.c b/drivers/soc/ti/omap_prm.c
>>> index 9b8d5945..fadfc7f 100644
>>> --- a/drivers/soc/ti/omap_prm.c
>>> +++ b/drivers/soc/ti/omap_prm.c
>>> @@ -73,8 +73,25 @@ struct omap_prm_data omap4_prm_data[] = {
>>>       { },
>>>   };
>>>   +struct omap_rst_map am3_wkup_rst_map[] = {
>>> +    { .rst = 3, .st = 5 },
>>> +    { .rst = -1 },
>>> +};
>>> +
>>> +struct omap_prm_data am3_prm_data[] = {
>>> +    { .name = "per", .base = 0x44e00c00, .pwstctrl = 0xc, .pwstst =
>>> 0x8, .flags = OMAP_PRM_NO_RSTST },
>>> +    { .name = "wkup", .base = 0x44e00d00, .pwstctrl = 0x4, .pwstst =
>>> 0x8, .rstst = 0xc, .rstmap = am3_wkup_rst_map },
>>> +    { .name = "mpu", .base = 0x44e00e00, .pwstst = 0x4 },
>>
>> Has a rstst but no rstctrl, but your registration logic takes care of
>> this. Somewhat confusing, when you just look at the data. Should you
>> limit the check to only rstctrl and OMAP_PRM_NO_RSTST?
> 
> I think its probably better I invert the flags and explicitly state
> OMAP_PRM_HAS_RSTST | OMAP_PRM_HAS_RSTCTRL, in case any zero value is
> used for these.

Yeah, something similar to HWMOD_OMAP4_ZERO_CLKCTRL_OFFSET in current
hwmod code.

> 
>>
>>> +    { .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst =
>>> 0x8 },
>>
>> No pwrstctrl and pwrstst registers, so same comment as on OMAP4 data.
> 
> I should probably add some flag for this in future once the support for
> power domains is added.
> 
> Anyway, I'll ditch all pwstctrl / pwstst data for now as it seems to
> bother you too much.

OK, that's probably cleaner, and the code and data can be handled when
you implement the power-domain pieces.

regards
Suman

> 
> -Tero
> 
>>
>>> +    { .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
>>> +    { .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl =
>>> 0x4, .rstst = 0x14 },
>>> +    { .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
>>
>> I am not sure if it is better to explicitly list the registers at 0
>> offset rather than using the implied value of 0, since there are some
>> registers that do not exist on some PRM instances which are also not
>> defined.
>>
>> regards
>> Suman
>>
>>> +    { },
>>> +};
>>> +
>>>   static const struct of_device_id omap_prm_id_table[] = {
>>>       { .compatible = "ti,omap4-prm-inst", .data = omap4_prm_data },
>>> +    { .compatible = "ti,am3-prm-inst", .data = am3_prm_data },
>>>       { },
>>>   };
>>>  
>>
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

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