pwm: add MACRO to limit some code which only for rk3288
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/pwm/rk_pwm.c +8 -0
@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h>
@@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; }
Signature · 1 line
1.9.1
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
Superfluous Change-Id.
Quoted 54 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +8
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; }
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
In reply to Kever YangHi Kever,
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Quoted 31 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,
drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice *dev) · line 77
Quoted 9 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif
I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like grf_enable_pwm() in the core SoC code and call it here. The #ifdef can be there.
Or perhaps we should have a general soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide a soc_enable_pwm() function to enable the pwm.
What do you think?
Quoted 15 lines
drivers/pwm/rk_pwm.c +2
-1return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; } --
Quoted 3 lines
1.9.1
Regards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi Simon,
Quoted 9 lines · Simon Glass
Hi Kever,
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 45 lines
Quoted 32 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice *dev) · line 77
Quoted 9 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.
Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file like rk_pwm.c.
For these system control registers, I'm sure the dedicate general soc.c is needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a general soc.c won't help much except all the system control are gather in one file .
I think the GRF setting is part of the module and driver, so we can leave it as it is,
and a simple syscon driver is enough for grf like what is kernel do.
Thanks,
- Kever
Quoted 23 lines
Quoted 15 lines
drivers/pwm/rk_pwm.c +2
-1return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; } --Quoted 3 lines
1.9.1
Regards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi Kever,
Quoted 79 lines · Kever Yang
Hi Simon,
Quoted 5 lines · Simon Glass
Hi Kever,
Quoted 1 line · Kever Yang
Quoted 7 lines
Quoted 5 lines
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 48 lines
Quoted 33 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice · line 78
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.
Quoted 8 lines
For these system control registers, I'm sure the dedicate general
soc.cis needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a generalsoc.cwon't help much except all the system control are gather in one file .I think the GRF setting is part of the module and driver, so we can leave it as it is,
and a simple syscon driver is enough for grf like what is kernel do.
I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Quoted 29 lines
Thanks,
- KeverQuoted 23 lines
Quoted 15 lines
drivers/pwm/rk_pwm.c +2
-1return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; } --Quoted 3 lines
1.9.1
Regards,
Simon
Regards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi Simon,
CC Doug for this topic.
Quoted 80 lines · Simon Glass
Hi Kever,
Quoted 77 lines · Kever Yang
Hi Simon,
Quoted 9 lines · Simon Glass
Hi Kever,
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 46 lines
Quoted 32 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice · line 78
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.For these system control registers, I'm sure the dedicate general
soc.cis needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a generalsoc.cwon't help much except all the system control are gather in one file .
Quoted 6 lines
Quoted 3 lines
I think the GRF setting is part of the module and driver, so we can leave it as it is,
and a simple syscon driver is enough for grf like what is kernel do.I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Doug(in cc list) send the patch for this pwm setting, but it seems does not accept by upstream,
I think people also don't like this grf access in driver and prefer this kind of one time init operation
to be done in bootloader.
patchset 1 implement in arch/arm/mach-rockchip/rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.html
patchset 4 implement in drivers/pwm_rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.html
Hi Doug,
Do you have any suggestion here?
Thanks,
- Kever
Quoted 32 lines
Quoted 27 lines
Thanks,
- KeverQuoted 22 lines
Quoted 14 lines
drivers/pwm/rk_pwm.c +2
-1return 0 ; }static int rk_pwm_probe (struct udevice *dev) { +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk_pwm_priv *priv =dev_get_priv (dev);rk_setreg (&priv->grf->soc_con2,1 <<0 ); +#endif return 0 ; } --Quoted 3 lines
1.9.1
Regards,
SimonRegards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi Kever,
Quoted 114 lines · Kever Yang
Hi Simon,
CC Doug for this topic.
Quoted 89 lines · Simon Glass
Hi Kever,
Quoted 85 lines · Kever Yang
Hi Simon,
Quoted 5 lines · Simon Glass
Hi Kever,
Quoted 1 line · Kever Yang
Quoted 7 lines
Quoted 5 lines
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 47 lines
Quoted 32 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice · line 78
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.For these system control registers, I'm sure the dedicate general
soc.cis
needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a generalsoc.cwon't help much except all the system control are gather in one file .Quoted 8 lines
Quoted 4 lines
I think the GRF setting is part of the module and driver, so we can leave it
as it is,
and a simple syscon driver is enough for grf like what is kernel do.I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Doug(in cc list) send the patch for this pwm setting, but it seems does not accept by upstream,
I think people also don't like this grf access in driver and prefer this kind of one time init operation
to be done in bootloader.
patchset 1 implement inarch/arm/mach-rockchip/rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.html
patchset 4 implement indrivers/pwm_rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.html
Quoted 2 lines
Hi Doug,
Do you have any suggestion here?
I think the device_is_compatible() idea would work OK for U-Boot if you want to do that. It's the CONFIG I want to avoid in the driver.
Regards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi Simon,
Quoted 89 lines · Simon Glass
Hi Kever,
Quoted 86 lines · Kever Yang
Hi Simon,
CC Doug for this topic.
Quoted 80 lines · Simon Glass
Hi Kever,
Quoted 77 lines · Kever Yang
Hi Simon,
Quoted 9 lines · Simon Glass
Hi Kever,
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 46 lines
Quoted 32 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice · line 78
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.For these system control registers, I'm sure the dedicate general
soc.cis
needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a generalsoc.cwon't help much except all the system control are gather in one file .
Which entry for soc init in general soc.c is reasonable? I prefer as early as possible,
because I want to do the clock init at the same place, how about board_early_init_f()
or arch_cpu_init()?
Quoted 18 lines
Quoted 18 lines
Quoted 8 lines
Quoted 5 lines
I think the GRF setting is part of the module and driver, so we can leave it
as it is,
and a simple syscon driver is enough for grf like what is kernel do.I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Doug(in cc list) send the patch for this pwm setting, but it seems does not accept by upstream,
I think people also don't like this grf access in driver and prefer this kind of one time init operation
to be done in bootloader.
patchset 1 implement inarch/arm/mach-rockchip/rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.html
patchset 4 implement indrivers/pwm_rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.html
Quoted 4 lines
Quoted 2 lines
Hi Doug,
Do you have any suggestion here?I think the
device_is_compatible()idea would work OK for U-Boot if you want to do that. It's the CONFIG I want to avoid in the driver.
The compatible in rk3399.dtsi for pwm is:
compatible = "rockchip,rk3399-pwm", "rockchip,rk3288-pwm";
So I would like to consider this setting as a SoC general setting in a SoC file,
instead of setting in pwm driver, because there might be other setting for other
module need us to fix in soc.c.
Thanks,
- Kever
Quoted 6 lines
Regards,
Simon
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
In reply to Kever YangHi,
Quoted 114 lines · Kever Yang
Hi Simon,
CC Doug for this topic.
Quoted 89 lines · Simon Glass
Hi Kever,
Quoted 85 lines · Kever Yang
Hi Simon,
Quoted 5 lines · Simon Glass
Hi Kever,
Quoted 1 line · Kever Yang
Quoted 7 lines
Quoted 5 lines
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 47 lines
Quoted 32 lines
Signed-off-by Kever Yang <[email protected]>--- drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif }; static int rk_pwm_set_config(struct udevice *dev, uint channel, uint
period_ns,drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice · line 78
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(struct udevice *dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr (dev); +#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.For these system control registers, I'm sure the dedicate general
soc.cis
needed, because they are not so common for different module and different Soc. We are not able to have a common framework for them, a generalsoc.cwon't help much except all the system control are gather in one file .Quoted 8 lines
Quoted 4 lines
I think the GRF setting is part of the module and driver, so we can leave it
as it is,
and a simple syscon driver is enough for grf like what is kernel do.I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Doug(in cc list) send the patch for this pwm setting, but it seems does not accept by upstream,
I think people also don't like this grf access in driver and prefer this kind of one time init operation
to be done in bootloader.
patchset 1 implement inarch/arm/mach-rockchip/rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.html
patchset 4 implement indrivers/pwm_rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.html
Quoted 2 lines
Hi Doug,
Do you have any suggestion here?
Heiko will skin me for suggesting it, but one option would be to consider this as a pinmux thing in the linux kernel. The GRF can choose between two IP blocks internally: the old PWM IP block or the new PWM IP block. ;)
I believe the other option is to handle atop Heiko's patches:
9131959 New [1/3] dt-bindings: add used but undocumented
rockchip grf compatible values
9131963 New [2/3] soc: rockchip: add driver handling grf setup
9131957 New [3/3] ARM: rockchip: drop rk3288 jtag/mmc switch handling
I believe Heiko's patches implement the "grf subclass" talked about in
<https://patchwork.kernel.org/patch/4738311/>.
-Doug
PATCH 1/1 pwm: add MACRO to limit some code which only for rk3288
Hi,
Am Donnerstag, 28. Juli 2016, 21:46:04 schrieb Doug Anderson:
Quoted 1 line
On Mon, Jul 11, 2016 at 7:45 PM, Kever Yang <[email protected]>
Quoted 134 lines · wrote:
Quoted 134 lines
Hi Simon,
CC Doug for this topic.
Quoted 113 lines · Simon Glass
Hi Kever,
Quoted 104 lines · Kever Yang
Hi Simon,
Quoted 10 lines · Simon Glass
Hi Kever,
Quoted 5 lines · Kever Yang
The grf setting for rkpwm is only need in rk3288, other SoCs like RK3399 which also use rkpwm do not need set the grf, let's add a MACRO to make the code only for RK3288.
Change-Id: I167a4e8cf925e840d4bbbcfb1437aaed52b81477
patman will automatically remove these for you.
Will use patman for my new patches later, thanks.
Quoted 67 lines
Quoted 52 lines
Signed-off-by Kever Yang <[email protected]>---
drivers/pwm/rk_pwm.c | 8 ++++++++ 1 file changed, 8 insertions(+)drivers/pwm/rk_pwm.c +4
-0@@ -13,8 +13,10 @@#include <syscon.h> #include <asm/io.h> #include <asm/arch/clock.h> +#ifdef CONFIG_ROCKCHIP_RK3288 #include <asm/arch/cru_rk3288.h> #include <asm/arch/grf_rk3288.h> +#endif #include <asm/arch/pwm.h> #include <power/regulator.h> @@ -22,7 +24,9 @@ DECLARE_GLOBAL_DATA_PTR;struct rk_pwm_priv {struct rk3288_pwm *regs; +#ifdef CONFIG_ROCKCHIP_RK3288 struct rk3288_grf *grf; +#endif };static int rk_pwm_set_config (struct udevice *dev,uint channel, uint
period_ns,drivers/pwm/rk_pwm.c +0
-0@@ -65,19 +69,23 @@ static int rk_pwm_ofdata_to_platdata(structudevice
*dev)struct regmap *map; priv->regs = (struct rk3288_pwm *)dev_get_addr(dev);drivers/pwm/rk_pwm.c static int rk_pwm_ofdata_to_platdata(struct udevice *dev) · line 88
Quoted 10 lines
drivers/pwm/rk_pwm.c +2
-0+#ifdef CONFIG_ROCKCHIP_RK3288 map =syscon_get_regmap_by_driver_data (ROCKCHIP_SYSCON_GRF );if (IS_ERR (map))return PTR_ERR (map); priv->grf =regmap_get_range (map,0 ); +#endif I'd like to find a better way. Do all chips have a grf? If so then perhaps we can have a function like
grf_enable_pwm()in the core SoC code and call it here. The #ifdef can be there.Quoted 5 lines
Or perhaps we should have a general
soc.c, with its own struct containing pointers to grf, sgrf, etc. It can be set up at the start, and then provide asoc_enable_pwm()function to enable the pwm.What do you think?
Every Rockchip soc have grf, and maybe sgrf, pmugrf which much like grf. The content in grf are very different for different SoC, it gathers all kinds of system/module control registers .
Back to the grf setting for pwm, this control operation is only need in RK3288, not in RK3399 and new SoC further. so I think the '#ifdef' is better to stay in driver file likerk_pwm.c.For these system control registers, I'm sure the dedicate general
soc.cis
needed, because they are not so common for different module and different
Soc. We are not able to have a common framework for them, a generalsoc.c
won't help much except all the system control are gather in one file .Quoted 5 lines
I think the GRF setting is part of the module and driver, so we can leave
it
as it is,
and a simple syscon driver is enough for grf like what is kernel do.Quoted 3 lines
I looked quickly at the Linux pwm driver but I don't see any grf access there. How does the grf register get set in Linux? I really want to avoid SoC-specific #ifdefs in drivers.
Doug(in cc list) send the patch for this pwm setting, but it seems does not
accept by upstream,
I think people also don't like this grf access in driver and prefer this kind of one time init operation
to be done in bootloader.
patchset 1 implement inarch/arm/mach-rockchip/rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280064.h
tml patchset 4 implement indrivers/pwm_rockchip.c
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-August/280715.h
tml
Quoted 8 lines
Quoted 3 lines
Hi Doug,
Do you have any suggestion here?
Heiko will skin me for suggesting it, but one option would be to consider this as a pinmux thing in the linux kernel. The GRF can choose between two IP blocks internally: the old PWM IP block or the new PWM IP block. ;)
somehow this mail slipped through the cracks in my inbox :-( .
But anyway, compared to the uart-discussion we're having elsewhere, I can somehow see how this might be viewed as pinctrl, simply because it has the funnel-structure, pin-muxing implies:
dw_pwm - \
--- pwm-pinmux - \rk_pwm - / -- pin
other pinfunc - /
So no skinning here ;-)
Quoted 10 lines
I believe the other option is to handle atop Heiko's patches:
9131959 New [1/3] dt-bindings: add used but undocumented
rockchip grf compatible values
9131963 New [2/3] soc: rockchip: add driver handling grf setup
9131957 New [3/3] ARM: rockchip: drop rk3288 jtag/mmc switch
handlingI believe Heiko's patches implement the "grf subclass" talked about in
<https://patchwork.kernel.org/patch/4738311/>.
considering that the two pwm, i2c etc implementations are mainly to have a fallback if the new block proves faulty (aka use rk_pwm by default and only fall back to the old one if it proves to have some unfixable error) I'd really consider this a init operation that should just be done one time.
Heiko