| Message ID | 1445117328-31678-1-git-send-email-mw@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Oct 17, 2015 at 11:28:48PM +0200, Marcin Wojtas wrote: > diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c > index 6ec82c6..094cb48 100644 > --- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c > +++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c > @@ -23,6 +23,7 @@ > #include "pinctrl-mvebu.h" > > static void __iomem *mpp_base; > +static u32 *mpp_saved_regs; I'm not a fan of unnecessary global variables. It adds to the bulk of the kernel when built-in (even though it's in .bss, it still has a cost) and these all add up when built-in. Please make it part of the driver data allocated at probe time. > static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config) > { > @@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) > const struct of_device_id *match = > of_match_device(armada_38x_pinctrl_of_match, &pdev->dev); > struct resource *res; > + int nregs; > > if (!match) > return -ENODEV; > @@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) > soc->modes = armada_38x_mpp_modes; > soc->nmodes = armada_38x_mpp_controls[0].npins; > > + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); > + > + mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32), > + GFP_KERNEL); > + if (!mpp_saved_regs) > + return -ENOMEM; > + > pdev->dev.platform_data = soc; The 'soc' is stored in platform data, not driver data, but... > > return mvebu_pinctrl_probe(pdev); > } > > +int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); You access it through driver data. Isn't the driver data here a struct mvebu_pinctrl pointer? See platform_set_drvdata() in mvebu_pinctrl_probe(). > + int i, nregs; > + > + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); > + > + for (i = 0; i < nregs; i++) > + mpp_saved_regs[i] = readl(mpp_base + i * 4); > + > + return 0; > +} > + > +int armada_38x_pinctrl_resume(struct platform_device *pdev) > +{ > + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); Ditto.
Hi Russell, Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a fix then, too) and it worked. I must have missed, because I got proper registers' number and values in suspend/resume routines. As pinctrl-armada-xp.c needs also a small fix and in order not to duplicate code, how about a following solution: - *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info - common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c (now there will be two users AXP and A38X) Please let me know what you think. Best regards, Marcin 2015-10-18 0:29 GMT+02:00 Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Sat, Oct 17, 2015 at 11:28:48PM +0200, Marcin Wojtas wrote: >> diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c >> index 6ec82c6..094cb48 100644 >> --- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c >> +++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c >> @@ -23,6 +23,7 @@ >> #include "pinctrl-mvebu.h" >> >> static void __iomem *mpp_base; >> +static u32 *mpp_saved_regs; > > I'm not a fan of unnecessary global variables. It adds to the bulk of > the kernel when built-in (even though it's in .bss, it still has a cost) > and these all add up when built-in. > > Please make it part of the driver data allocated at probe time. > >> static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config) >> { >> @@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) >> const struct of_device_id *match = >> of_match_device(armada_38x_pinctrl_of_match, &pdev->dev); >> struct resource *res; >> + int nregs; >> >> if (!match) >> return -ENODEV; >> @@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) >> soc->modes = armada_38x_mpp_modes; >> soc->nmodes = armada_38x_mpp_controls[0].npins; >> >> + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); >> + >> + mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32), >> + GFP_KERNEL); >> + if (!mpp_saved_regs) >> + return -ENOMEM; >> + >> pdev->dev.platform_data = soc; > > The 'soc' is stored in platform data, not driver data, but... > >> >> return mvebu_pinctrl_probe(pdev); >> } >> >> +int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state) >> +{ >> + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); > > You access it through driver data. Isn't the driver data here a > struct mvebu_pinctrl pointer? See platform_set_drvdata() in > mvebu_pinctrl_probe(). > >> + int i, nregs; >> + >> + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); >> + >> + for (i = 0; i < nregs; i++) >> + mpp_saved_regs[i] = readl(mpp_base + i * 4); >> + >> + return 0; >> +} >> + >> +int armada_38x_pinctrl_resume(struct platform_device *pdev) >> +{ >> + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); > > Ditto. > > -- > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
Hello Marcin, On Sun, 18 Oct 2015 10:43:42 +0200, Marcin Wojtas wrote: > Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a > fix then, too) and it worked. I must have missed, because I got proper > registers' number and values in suspend/resume routines. As > pinctrl-armada-xp.c needs also a small fix and in order not to > duplicate code, how about a following solution: > - *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info I don't like this. The mvebu_pinctrl_soc_info structure is meant to be a read-only structure that only describes static information giving SoC-specific details for pin-muxing. The idea is that in the event where you had multiple pinctrl in the same system, you would still have only one instance of mvebu_pinctrl_soc_info. So, I'd prefer if mpp_saved_regs and mpp_base became members of a new structure, that gets allocated at ->probe() time, on a per-instance basis. > - common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c > (now there will be two users AXP and A38X) Not sure how to handle that if the per-instance structure is defined on a per-SoC basis, but I'm interested in seeing proposals. Best regards, Thomas
Hi Thomas, 2015-10-18 16:01 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Hello Marcin, > > On Sun, 18 Oct 2015 10:43:42 +0200, Marcin Wojtas wrote: > >> Thanks for pointing this. I based on pinctrl-armada-xp.c (it needs a >> fix then, too) and it worked. I must have missed, because I got proper >> registers' number and values in suspend/resume routines. As >> pinctrl-armada-xp.c needs also a small fix and in order not to >> duplicate code, how about a following solution: >> - *mpp_saved_regs and *mpp_base become members of struct mvebu_pinctrl_soc_info > > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be > a read-only structure that only describes static information giving > SoC-specific details for pin-muxing. The idea is that in the event > where you had multiple pinctrl in the same system, you would still have > only one instance of mvebu_pinctrl_soc_info. Ok, understood. What with current static globals, like mpp_base? This is a problem when we consider hypothetical multi-pintrl system... > > So, I'd prefer if mpp_saved_regs and mpp_base became members of a new > structure, that gets allocated at ->probe() time, on a per-instance > basis. > >> - common mvebu_pinctrl_suspend/resume functions in pinctrl-mvebu.c >> (now there will be two users AXP and A38X) > > Not sure how to handle that if the per-instance structure is defined on > a per-SoC basis, but I'm interested in seeing proposals. > In genereal, I think storing additional global data is not starightforward, as dev->platform_data and dev->driver_data are currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I propose the following: 1. Create a new structure: struct mvebu_pinctrl_pm_info { void __iomem *base; static u32 *mpp_saved_regs; int nregs; } 2. Add new field to struct mvebu_pinctrl: struct mvebu_pinctrl_pm_info *pm_info; 3. In armada_38x/xp_pinctrl_probe we do the allocations and pass struct pm_info using dev->driver data (later in mvebu_pinctrl_probe it will be replaced by struct mvebu_pinctrl): platform_set_drvdata(pdev, pm_info); return mvebu_pinctrl_probe(pdev); 4. In mvebu_pinctrl_probe: struct mvebu_pinctrl_pm_info *pm_info = platform_get_drvdata(pdev); (...) if (pm_info) pctl->pm_info = pm_info; platform_set_drvdata(pdev, pctl); 5. Now, we can simply use all stored data in common mvebu_pinctrl_suspend/resume. I hope the above is clear. I'm looking forward to your opinion. Beste regards, Marcin
Hello, On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote: > > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be > > a read-only structure that only describes static information giving > > SoC-specific details for pin-muxing. The idea is that in the event > > where you had multiple pinctrl in the same system, you would still have > > only one instance of mvebu_pinctrl_soc_info. > > Ok, understood. What with current static globals, like mpp_base? This > is a problem when we consider hypothetical multi-pintrl system... The current driver is indeed not designed for multiple instances of the same pinctrl controller. But that's exactly what Russell is asking for. > In genereal, I think storing additional global data is not > starightforward, as dev->platform_data and dev->driver_data are > currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I > propose the following: > > > 1. Create a new structure: > struct mvebu_pinctrl_pm_info { This definitely shouldn't be called "pm_info", because 'base' is not PM related. It should be mvebu_pinctrl_state or something like that. > void __iomem *base; > static u32 *mpp_saved_regs; > int nregs; > } > > 2. Add new field to struct mvebu_pinctrl: > struct mvebu_pinctrl_pm_info *pm_info; Does not work because "mvebu_pinctrl_pm_info" cannot be a generic structure, it has to be a per-SoC driver structure, since the set of registers to save for PM reasons is different from one SoC to the other. Also, some SoC have only one "base" pointer, some others (like Dove) have multiple. So it should be the other way around: the SoC-specific driver create a structure, and this structure points back to the mvebu_pinctrl structure. Best regards, Thomas
Thomas, 2015-10-19 9:23 GMT+02:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Hello, > > On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote: > >> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be >> > a read-only structure that only describes static information giving >> > SoC-specific details for pin-muxing. The idea is that in the event >> > where you had multiple pinctrl in the same system, you would still have >> > only one instance of mvebu_pinctrl_soc_info. >> >> Ok, understood. What with current static globals, like mpp_base? This >> is a problem when we consider hypothetical multi-pintrl system... > > The current driver is indeed not designed for multiple instances of the > same pinctrl controller. But that's exactly what Russell is asking for. > In each mvebu pinctl there are <soc>_mpp_ctrl_get/set force usage of global mpp_base. In order not to use it this way the functions have to be redesigned. IMO having an acces to pdev would be sufficient (please see my proposal below), but yet I don't have a clear idea, how to do it, >> In genereal, I think storing additional global data is not >> starightforward, as dev->platform_data and dev->driver_data are >> currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I >> propose the following: >> >> >> 1. Create a new structure: >> struct mvebu_pinctrl_pm_info { > > This definitely shouldn't be called "pm_info", because 'base' is not PM > related. It should be mvebu_pinctrl_state or something like that. > >> void __iomem *base; >> static u32 *mpp_saved_regs; >> int nregs; >> } >> >> 2. Add new field to struct mvebu_pinctrl: >> struct mvebu_pinctrl_pm_info *pm_info; > > Does not work because "mvebu_pinctrl_pm_info" cannot be a generic > structure, it has to be a per-SoC driver structure, since the set of > registers to save for PM reasons is different from one SoC to the > other. Also, some SoC have only one "base" pointer, some others (like > Dove) have multiple. > > So it should be the other way around: the SoC-specific driver create a > structure, and this structure points back to the mvebu_pinctrl > structure. > How about a following compromise: Instead of forcing pm_info, we can add a generic pointer to struct mvebu_pinctrl, e.g. void *priv; and enable passing it in a way I proposed before. This way each SoC-specific driver can attach whatever is needed for its operation. For example in A38X/AXP it would be *base, *mpp_saved_regs and nregs gathered in one structure, but it wouldn't force anything specific for other machines. As a result in A38X/AXP in suspend/resume routines, there would be no globals in use at all. Best regards, Marcin
Hi Thomas, Found it - I think there is an easy way to get rid of all global variables in each pinctrl-<soc>. It's enough to: - extend struct mvebu_pinctrl with generic pointer - pass SoC specific structure to mvebu_pinctrl_probe via dev->driver_data - in mvebu_pinconf_group_set/get pass an additional argument to mpp_set/get: struct mvebu_pinctrl *pctl. - update mpp_set/get callbacks in each driver accordingly This way in SoC-specific pinctrl driver we determine what data we want to use and how. I can prepare a patchset with all described changes. I'm looking forward to your feedback. Best regards, Marcin 2015-10-19 11:25 GMT+02:00 Marcin Wojtas <mw@semihalf.com>: > Thomas, > > 2015-10-19 9:23 GMT+02:00 Thomas Petazzoni > <thomas.petazzoni@free-electrons.com>: >> Hello, >> >> On Mon, 19 Oct 2015 08:04:49 +0200, Marcin Wojtas wrote: >> >>> > I don't like this. The mvebu_pinctrl_soc_info structure is meant to be >>> > a read-only structure that only describes static information giving >>> > SoC-specific details for pin-muxing. The idea is that in the event >>> > where you had multiple pinctrl in the same system, you would still have >>> > only one instance of mvebu_pinctrl_soc_info. >>> >>> Ok, understood. What with current static globals, like mpp_base? This >>> is a problem when we consider hypothetical multi-pintrl system... >> >> The current driver is indeed not designed for multiple instances of the >> same pinctrl controller. But that's exactly what Russell is asking for. >> > > In each mvebu pinctl there are <soc>_mpp_ctrl_get/set force usage of > global mpp_base. In order not to use it this way the functions have to > be redesigned. IMO having an acces to pdev would be sufficient (please > see my proposal below), but yet I don't have a clear idea, how to do > it, > >>> In genereal, I think storing additional global data is not >>> starightforward, as dev->platform_data and dev->driver_data are >>> currently occupied by mvebu_pinctrl and mvebu_pinctrl_soc_info. I >>> propose the following: >>> >>> >>> 1. Create a new structure: >>> struct mvebu_pinctrl_pm_info { >> >> This definitely shouldn't be called "pm_info", because 'base' is not PM >> related. It should be mvebu_pinctrl_state or something like that. >> >>> void __iomem *base; >>> static u32 *mpp_saved_regs; >>> int nregs; >>> } >>> >>> 2. Add new field to struct mvebu_pinctrl: >>> struct mvebu_pinctrl_pm_info *pm_info; >> >> Does not work because "mvebu_pinctrl_pm_info" cannot be a generic >> structure, it has to be a per-SoC driver structure, since the set of >> registers to save for PM reasons is different from one SoC to the >> other. Also, some SoC have only one "base" pointer, some others (like >> Dove) have multiple. >> >> So it should be the other way around: the SoC-specific driver create a >> structure, and this structure points back to the mvebu_pinctrl >> structure. >> > > How about a following compromise: > Instead of forcing pm_info, we can add a generic pointer to struct > mvebu_pinctrl, e.g. void *priv; and enable passing it in a way I > proposed before. > > This way each SoC-specific driver can attach whatever is needed for > its operation. For example in A38X/AXP it would be *base, > *mpp_saved_regs and nregs gathered in one structure, but it wouldn't > force anything specific for other machines. As a result in A38X/AXP in > suspend/resume routines, there would be no globals in use at all. > > Best regards, > Marcin
diff --git a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c index 6ec82c6..094cb48 100644 --- a/drivers/pinctrl/mvebu/pinctrl-armada-38x.c +++ b/drivers/pinctrl/mvebu/pinctrl-armada-38x.c @@ -23,6 +23,7 @@ #include "pinctrl-mvebu.h" static void __iomem *mpp_base; +static u32 *mpp_saved_regs; static int armada_38x_mpp_ctrl_get(unsigned pid, unsigned long *config) { @@ -424,6 +425,7 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) const struct of_device_id *match = of_match_device(armada_38x_pinctrl_of_match, &pdev->dev); struct resource *res; + int nregs; if (!match) return -ENODEV; @@ -441,11 +443,44 @@ static int armada_38x_pinctrl_probe(struct platform_device *pdev) soc->modes = armada_38x_mpp_modes; soc->nmodes = armada_38x_mpp_controls[0].npins; + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); + + mpp_saved_regs = devm_kcalloc(&pdev->dev, nregs, sizeof(u32), + GFP_KERNEL); + if (!mpp_saved_regs) + return -ENOMEM; + pdev->dev.platform_data = soc; return mvebu_pinctrl_probe(pdev); } +int armada_38x_pinctrl_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); + int i, nregs; + + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); + + for (i = 0; i < nregs; i++) + mpp_saved_regs[i] = readl(mpp_base + i * 4); + + return 0; +} + +int armada_38x_pinctrl_resume(struct platform_device *pdev) +{ + struct mvebu_pinctrl_soc_info *soc = platform_get_drvdata(pdev); + int i, nregs; + + nregs = DIV_ROUND_UP(soc->nmodes, MVEBU_MPPS_PER_REG); + + for (i = 0; i < nregs; i++) + writel(mpp_saved_regs[i], mpp_base + i * 4); + + return 0; +} + static int armada_38x_pinctrl_remove(struct platform_device *pdev) { return mvebu_pinctrl_remove(pdev); @@ -458,6 +493,8 @@ static struct platform_driver armada_38x_pinctrl_driver = { }, .probe = armada_38x_pinctrl_probe, .remove = armada_38x_pinctrl_remove, + .suspend = armada_38x_pinctrl_suspend, + .resume = armada_38x_pinctrl_resume, }; module_platform_driver(armada_38x_pinctrl_driver);
This commit adds suspend/resume support by saving and restoring registers' state in a dedicated array. Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- drivers/pinctrl/mvebu/pinctrl-armada-38x.c | 37 ++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)