diff mbox series

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

Message ID 1565164139-21886-7-git-send-email-t-kristo@ti.com (mailing list archive)
State New, archived
Headers show
Series soc: ti: Add OMAP PRM driver | expand

Commit Message

Tero Kristo Aug. 7, 2019, 7:48 a.m. UTC
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(+)

Comments

Suman Anna Aug. 19, 2019, 11:11 p.m. UTC | #1
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[] = {

static const here and below.

regards
Suman

> +	{ .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 },
> +	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },
> +	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
> +	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
> +	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
> +	{ },
> +};
> +
>  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 },
>  	{ },
>  };
>  
>
Suman Anna Aug. 20, 2019, 6:48 p.m. UTC | #2
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?

> +	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },

No pwrstctrl and pwrstst registers, so same comment as on OMAP4 data.

> +	{ .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 },
>  	{ },
>  };
>  
>
Tero Kristo Aug. 21, 2019, 7:23 a.m. UTC | #3
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.

> 
>> +	{ .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.

-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
Suman Anna Aug. 21, 2019, 3:49 p.m. UTC | #4
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
diff mbox series

Patch

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 },
+	{ .name = "device", .base = 0x44e00f00, .rstctl = 0x0, .rstst = 0x8 },
+	{ .name = "rtc", .base = 0x44e01000, .pwstst = 0x4 },
+	{ .name = "gfx", .base = 0x44e01100, .pwstst = 0x10, .rstctl = 0x4, .rstst = 0x14 },
+	{ .name = "cefuse", .base = 0x44e01200, .pwstst = 0x4 },
+	{ },
+};
+
 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 },
 	{ },
 };