| Message ID | 5737609.o5tyv6lkQG@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Heiko, On Thursday 01 May 2014 at 15:21:34, Heiko Stübner wrote: > Hi Max, > > Am Donnerstag, 1. Mai 2014, 12:43:13 schrieb Max Schwarz: > > On Wednesday 30 April 2014 at 00:07:12, Heiko Stübner wrote: > > > While this wasn't a problem until now, the upcoming rk3288 introduces > > > additional changes to both the grf and pmu areas. On it even part of > > > the pinmux registers move into the pmu space. > > > > Could you give us more information on that? I tried to find details on the > > RK3288 but came up with nothing. How are the pinmux registers divided? > > Some days ago, Rockchip released kernel sources for the rk3288 [0]. They > took a lot of our current mainline code as base for their kernel. AS you > can see in the register map below, the pinmux registers for the gpio0 bank > are residing in the pmu space, while gpio1-8 are residing in the regular > "general register files" Wow, that's interesting. Seems they invested some real effort to catch up instead of simply modifying their old patches. Which is a compliment to your contributions! Maybe we should try to get one of the Rockchip developers on board. They must have thought about this kind of thing as well. > To elaborate a bit: > > On rk3188 it is > GRF: 0x00 - 0x5c: pin suspend control > GRF: 0x60 - 0x9c: pinmux control (0x60 and 0x64 gpio-only) > GRF: 0xa0 - 0xac: soc-control/status > GRF: 0xb0 - 0xc8: dma-control > GRF: 0xcc - 0xe0: "cpu core configuration" > GRF: 0xec - 0xf0: ddr-controller config > GRF: 0xf4 - 0x104: pin drive-strength (what we currently do not support) > GRF: 0x108: soc_status1 > GRF: 0x10c - 0x140: USB phy control > GRF: 0x144 - 0x160: "OS register" > GRF: 0x160 - 0x19c: pin pull settings > PMU: 0x00 - 0x38: power-domains and a lot of unknown stuff > PMU: 0x3c: something called GPIO0_CON, what Rockchip does not use at all > PMU: 0x40 - 0x60: "SYS_REGx" > PMU: 0x64 - 0x68: part of GPIO0 pull config > > so we would/will in the end need 4 mappings for the rk3188-pinctrl > GRF: 0x00 - 0x9c, GRF: 0xf4 - 0x104, GRF: 0x160 - 0x19c, PMU: 0x64 - 0x68 > > > On rk3288 it is > > GRF: 0x00 - 0x84: gpio1-gpio8 iomux settings > GRF: 0x104 - 0x138: unknown GPIOxx_SR registers > GRF: 0x140 - 0x1b4: gpio1-gpio8 pull settings > GRF: 0x1c0 - 0x234: gpio1-gpio8 driver strength settings > GRF: 0x240: unknown/unused GPIO_SMT > GRF: 0x244 - 0x2d4: soc control/status registers > GRF: 0x2e0 - ...: a lot of stuff like dma, usb-phy etc. > PMU: 0x00 - 0x5c: powerdomains and a lot of other stuff > PMU: 0x60: GPIO_SR > PMU: 0x64 - 0x6c: gpio0 pull settings > PMU: 0x70 - 0x78: gpio0 drive-strength settings > PMU: 0x7c: GPIO_OP > PMU: 0x80: GPIO0_SEL18 > PMU: 0x84 - 0x8c: gpio0 pinmux settings > PMU: 0x90 - 0xa0: more misc registers (powermode, sys_regX) > > so we would essentially need only two mappings here > GRF: 0x00 - 0x240 and PMU: 0x60 - 0x8c > > So we'd need additional if(is_rk3188()) conditionals to distinguish between > these implementations [and possible future ones] to select the correct base > address, and we don't know what the next SoC will bring and how the stuff > will be ordered there. Thanks for providing the register mappings. Yes, if you do specify the mappings as you proposed it would be a nightmare. However, this sheds light on an underlying issue: Rockchip is not treating the whole GPIO block as one cohesive device as we do currently. Instead, it seems to me, one GPIO bank is one device. Each has its cohesive mux, bank and pull registers - apart from rk3188-bank-0, maybe. But that one is special anyway with regards to register ordering (s.b.). The issues you had with RK3188 and now have with RK3288 seem to stem from trying to group all banks together into one pinctrl driver. So maybe we should promote the GPIO banks to full devices in the dt and make smaller mappings for each GPIO bank, i.e. three mappings for each GPIO bank (mux, bank, pull). I know we have to stay backwards compatible dt-wise, but that should be doable. Then we are fully flexible and don't need any conditionals or address calculation logic. And should a future SoC bring another layout inside the banks, we can react with a new "compatible"-name (and maybe a completely separate driver, if the change is big enough). > Also leaving the driver behind, devicetree is meant to describe the > hardware, not the implementation. And hw-wise both PMU and GRF are actual > hardware blocks even with individual clock gates. Whatever a "hardware block" is. n:m mappings between devices and clocks are no problems in the dt. So why not describe things a bit more precisely? > Citing the syscon-devicetree bindings: > > System controller node represents a register region containing a set > of miscellaneous registers. The registers are not cohesive enough to > represent as any specific type of device. > > So to me both GRF and PMU regions scream "syscon". Yes, that sounds a bit like the mess we are dealing with ;-) I still feel that declaring everything as syscon is somehow circumventing the dt. And I feel more comfortable declaring GRF_SOC_* as "miscellaneous registers" rather than e.g. the iomux space of GPIO0. Don't get me wrong please, I'm not completely against the syscon idea. I'm just trying to have a full discussion on the issue. > I've attached my current WIP patch to implement rk3288 support (untested, as > I don't have any hardware), based on this series. As you can see in it, the > rk3288 has even more peculiarities with gpio-only and 4bit wide iomuxes. Nice, but you needed to introduce flags like "SOURCE_PMU/GRF" which would not be necessary with the fine-grained mapping. GPIO-Only could be handled by a mask specified in the dt. > As the patch stands now, rk3288 doesn't even need special handling for its > iomux registers, as it can be simply described now in the pin-bank > declaration at the bottom - and even the rk3188-bank0 wouldn't be necessary > anymore. /* * The bits in these registers have an inverse ordering * with the lowest pin being in bits 15:14 and the highest * pin in bits 1:0 */ *bit = 7 - (pin_num % RK3188_PULL_PINS_PER_REG); (from rk3188_calc_pull_reg_and_bit) That's probably still a peculiarity of rk3188-bank0, isn't it? So we'd still need a conditional on rk3188-bank0. That could enable a fourth mapping for the split up mux space of bank0 as well. Compared to your RK3288 patch we'd be moving the information from the table in your driver (which is just describing the hw layout & capabilities) into the dt. > > There are some question marks for me on the syscon solution. Regmap uses > > locking internally, which means separate drivers can't access separate > > registers simultaneously. We have an SMP machine here, so that's not far- > > fetched. And that locking is completely unnecessary, as we *know* in most > > cases that the accessed areas do not overlap. > > For locking vs. speed, I do not see this as a big problem. All registers in > there mainly contain general settings that are not changed often during the > operation of the device. So there is no high frequency access to them in any > case. Agreed, that's probably not an issue (if no one wants to do high-speed concurrent bitbang I/O :D). > > > The other option would be to leave the grf as it is and create separate > > > syscons for real small individual parts like the soc-conf and usb-phys. > > > But apart from creating these real small syscons that would > > > also make it necessary to introduce another register map for the > > > drive-strength settings of the pin-controller, which are sitting in the > > > middle of everything at least on rk3066 and rk3188. > > > > Wy do we need a syscon for usb-phys? Is it shared by multiple drivers? > > My instinctive approach would be two usb-phys devices mapping the > > GRF_UOC0/1 spaces directly via reg properties. Or did I miss something? > > Of course if we're going to map each part of the GRF individually there is > no need for a syscon. Okay, sorry for misunderstanding. > I'm also hoping for more input so I've changed the title a bit, to maybe > attract more people :-). Yes, let's hope someone else speaks up. Maybe there has already been a precedent in another mach-*? I'll try to find something similar. In the end you as the maintainer have to make the decision though. And as I said I don't have real problems with the syscon solution, it just doesn't feel nice. Cheers, Max
Hi Max, Am Freitag, 2. Mai 2014, 15:59:05 schrieb Max Schwarz: > On Thursday 01 May 2014 at 15:21:34, Heiko Stübner wrote: > > On rk3188 it is > > GRF: 0x00 - 0x5c: pin suspend control > > GRF: 0x60 - 0x9c: pinmux control (0x60 and 0x64 gpio-only) > > GRF: 0xa0 - 0xac: soc-control/status > > GRF: 0xb0 - 0xc8: dma-control > > GRF: 0xcc - 0xe0: "cpu core configuration" > > GRF: 0xec - 0xf0: ddr-controller config > > GRF: 0xf4 - 0x104: pin drive-strength (what we currently do not support) > > GRF: 0x108: soc_status1 > > GRF: 0x10c - 0x140: USB phy control > > GRF: 0x144 - 0x160: "OS register" > > GRF: 0x160 - 0x19c: pin pull settings > > PMU: 0x00 - 0x38: power-domains and a lot of unknown stuff > > PMU: 0x3c: something called GPIO0_CON, what Rockchip does not use at all > > PMU: 0x40 - 0x60: "SYS_REGx" > > PMU: 0x64 - 0x68: part of GPIO0 pull config > > > > so we would/will in the end need 4 mappings for the rk3188-pinctrl > > GRF: 0x00 - 0x9c, GRF: 0xf4 - 0x104, GRF: 0x160 - 0x19c, PMU: 0x64 - 0x68 > > > > > > On rk3288 it is > > > > GRF: 0x00 - 0x84: gpio1-gpio8 iomux settings > > GRF: 0x104 - 0x138: unknown GPIOxx_SR registers > > GRF: 0x140 - 0x1b4: gpio1-gpio8 pull settings > > GRF: 0x1c0 - 0x234: gpio1-gpio8 driver strength settings > > GRF: 0x240: unknown/unused GPIO_SMT > > GRF: 0x244 - 0x2d4: soc control/status registers > > GRF: 0x2e0 - ...: a lot of stuff like dma, usb-phy etc. > > PMU: 0x00 - 0x5c: powerdomains and a lot of other stuff > > PMU: 0x60: GPIO_SR > > PMU: 0x64 - 0x6c: gpio0 pull settings > > PMU: 0x70 - 0x78: gpio0 drive-strength settings > > PMU: 0x7c: GPIO_OP > > PMU: 0x80: GPIO0_SEL18 > > PMU: 0x84 - 0x8c: gpio0 pinmux settings > > PMU: 0x90 - 0xa0: more misc registers (powermode, sys_regX) > > > > so we would essentially need only two mappings here > > GRF: 0x00 - 0x240 and PMU: 0x60 - 0x8c > > > > So we'd need additional if(is_rk3188()) conditionals to distinguish > > between > > these implementations [and possible future ones] to select the correct > > base > > address, and we don't know what the next SoC will bring and how the stuff > > will be ordered there. > > Thanks for providing the register mappings. > > Yes, if you do specify the mappings as you proposed it would be a nightmare. > > However, this sheds light on an underlying issue: Rockchip is not treating > the whole GPIO block as one cohesive device as we do currently. Instead, it > seems to me, one GPIO bank is one device. Each has its cohesive mux, bank > and pull registers - apart from rk3188-bank-0, maybe. But that one is > special anyway with regards to register ordering (s.b.). > > The issues you had with RK3188 and now have with RK3288 seem to stem from > trying to group all banks together into one pinctrl driver. > > So maybe we should promote the GPIO banks to full devices in the dt and make > smaller mappings for each GPIO bank, i.e. three mappings for each GPIO bank > (mux, bank, pull). I know we have to stay backwards compatible dt-wise, but > that should be doable. Yes, that is another miss-conception from the early days. The gpio-controllers themself are actually Synopsis Designware IPs - the kernel now even has a separate driver for them (gpio-dwapb). Only the real pincontrol/muxing/pull/etc is from Rockchip. Currently I can't think of a way to move over gracefully, without introducing crap into the gpio-dwapb driver. As at the moment it parses all information it needs from the dt directly - of course with different bindings. That is also the reason I do not want to introduce more "special-cases" in the bank-declaration we're using currently - to not make this move more difficult in the future. As it is, with the patch attached to the last mail, the pinctrl driver wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as the information about its special case is now sitting in the bank declaration inside the driver. Which then would enable us to remove the gpio-bank subnodes alltogether and use the external gpio driver. > Then we are fully flexible and don't need any conditionals or address > calculation logic. And should a future SoC bring another layout inside the > banks, we can react with a new "compatible"-name (and maybe a completely > separate driver, if the change is big enough). Especially the rk3288 seems to introduce a lot more variants. So if we were to really split each pin-bank into a separate node, each bank would need to be handled differently - as they all have one peculiarity or another. > > Also leaving the driver behind, devicetree is meant to describe the > > hardware, not the implementation. And hw-wise both PMU and GRF are actual > > hardware blocks even with individual clock gates. > > Whatever a "hardware block" is. n:m mappings between devices and clocks are > no problems in the dt. So why not describe things a bit more precisely? > > Citing the syscon-devicetree bindings: > > System controller node represents a register region containing a set > > of miscellaneous registers. The registers are not cohesive enough to > > represent as any specific type of device. > > > > So to me both GRF and PMU regions scream "syscon". > > Yes, that sounds a bit like the mess we are dealing with ;-) > I still feel that declaring everything as syscon is somehow circumventing > the dt. And I feel more comfortable declaring GRF_SOC_* as "miscellaneous > registers" rather than e.g. the iomux space of GPIO0. > > Don't get me wrong please, I'm not completely against the syscon idea. I'm > just trying to have a full discussion on the issue. Oh, me too :-) . I'm not entirely fixated on this idea, but using a syscon feels somehow naturally to me in this case. While for example syscon cannot handle clocks currently, the underlying regmap can - so it would be only a matter of teaching syscon to tell regmap of the clock to use (GRF and PMU register-clocks in this case), instead of needing to have every user of parts of these registers handle the relevant clock itself on register accesses. Also, as you can see in the grf map, the rk3188 actually has two soc_status registers (0xac and 0x108) which have the dmac, cpu and drive strength registers in between. So having a syscon only for soc_con and soc_status will produce problems too. Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin voltage selection. As it only spans 1 register I'd assume it could contain settings that span all pin banks. And of course, splitting the register space into dozens of small individual mappings looks messy :-) > > I've attached my current WIP patch to implement rk3288 support (untested, > > as I don't have any hardware), based on this series. As you can see in > > it, the rk3288 has even more peculiarities with gpio-only and 4bit wide > > iomuxes. > Nice, but you needed to introduce flags like "SOURCE_PMU/GRF" which would > not be necessary with the fine-grained mapping. GPIO-Only could be handled > by a mask specified in the dt. While it is true, that on the rk3288 all gpio0-iomux settings are residing the in the pmu, this is not necessarily true for everything. Like on the rk3188, the iomux registers of bank0 are in the GRF space while parts of the pull settings are in the PMU space. > > As the patch stands now, rk3288 doesn't even need special handling for its > > iomux registers, as it can be simply described now in the pin-bank > > declaration at the bottom - and even the rk3188-bank0 wouldn't be > > necessary > > anymore. > > /* > * The bits in these registers have an inverse ordering > * with the lowest pin being in bits 15:14 and the highest > * pin in bits 1:0 > */ > *bit = 7 - (pin_num % RK3188_PULL_PINS_PER_REG); > > (from rk3188_calc_pull_reg_and_bit) > > That's probably still a peculiarity of rk3188-bank0, isn't it? So we'd still > need a conditional on rk3188-bank0. That could enable a fourth mapping for > the split up mux space of bank0 as well. No, the pull-pins are reversed inside the register on all banks of the rk3188. > > I'm also hoping for more input so I've changed the title a bit, to maybe > > attract more people :-). > > Yes, let's hope someone else speaks up. Maybe there has already been a > precedent in another mach-*? I'll try to find something similar. > In the end you as the maintainer have to make the decision though. And as I > said I don't have real problems with the syscon solution, it just doesn't > feel nice. Actually Linus Walleij is the pinctrl maintainer, so he'll also needs to be happy with what we come up here :-) . Heiko
Hello Heiko, On Saturday 03 May 2014 at 01:45:11, Heiko Stübner wrote: > > However, this sheds light on an underlying issue: Rockchip is not treating > > the whole GPIO block as one cohesive device as we do currently. Instead, > > it > > seems to me, one GPIO bank is one device. Each has its cohesive mux, bank > > and pull registers - apart from rk3188-bank-0, maybe. But that one is > > special anyway with regards to register ordering (s.b.). > > > > The issues you had with RK3188 and now have with RK3288 seem to stem from > > trying to group all banks together into one pinctrl driver. > > > > So maybe we should promote the GPIO banks to full devices in the dt and > > make smaller mappings for each GPIO bank, i.e. three mappings for each > > GPIO bank (mux, bank, pull). I know we have to stay backwards compatible > > dt-wise, but that should be doable. > > Yes, that is another miss-conception from the early days. The > gpio-controllers themself are actually Synopsis Designware IPs - the kernel > now even has a separate driver for them (gpio-dwapb). Only the real > pincontrol/muxing/pull/etc is from Rockchip. > > Currently I can't think of a way to move over gracefully, without > introducing crap into the gpio-dwapb driver. As at the moment it parses all > information it needs from the dt directly - of course with different > bindings. > > That is also the reason I do not want to introduce more "special-cases" in > the bank-declaration we're using currently - to not make this move more > difficult in the future. > > As it is, with the patch attached to the last mail, the pinctrl driver > wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as > the information about its special case is now sitting in the bank > declaration inside the driver. Which then would enable us to remove the > gpio-bank subnodes alltogether and use the external gpio driver. Okay, you convinced me. That sounds like a good plan to me. Maybe we can introduce some compability code into the pinctrl driver to generate the correct dt nodes for gpio-dwabp at runtime? I think that would depend on something like CONFIG_OF_DYNAMIC though. > While for example syscon cannot handle clocks currently, the underlying > regmap can - so it would be only a matter of teaching syscon to tell regmap > of the clock to use (GRF and PMU register-clocks in this case), instead of > needing to have every user of parts of these registers handle the relevant > clock itself on register accesses. > > Also, as you can see in the grf map, the rk3188 actually has two soc_status > registers (0xac and 0x108) which have the dmac, cpu and drive strength > registers in between. So having a syscon only for soc_con and soc_status > will produce problems too. > > Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin > voltage selection. As it only spans 1 register I'd assume it could contain > settings that span all pin banks. Okay, we couldn't handle that with individual devices for each bank. > And of course, splitting the register space into dozens of small individual > mappings looks messy :-) Yes, I agree now ;-) Cheers, Max
Am Samstag, 3. Mai 2014, 14:40:36 schrieb Max Schwarz: > Hello Heiko, > > On Saturday 03 May 2014 at 01:45:11, Heiko Stübner wrote: > > > However, this sheds light on an underlying issue: Rockchip is not > > > treating > > > the whole GPIO block as one cohesive device as we do currently. Instead, > > > it > > > seems to me, one GPIO bank is one device. Each has its cohesive mux, > > > bank > > > and pull registers - apart from rk3188-bank-0, maybe. But that one is > > > special anyway with regards to register ordering (s.b.). > > > > > > The issues you had with RK3188 and now have with RK3288 seem to stem > > > from > > > trying to group all banks together into one pinctrl driver. > > > > > > So maybe we should promote the GPIO banks to full devices in the dt and > > > make smaller mappings for each GPIO bank, i.e. three mappings for each > > > GPIO bank (mux, bank, pull). I know we have to stay backwards compatible > > > dt-wise, but that should be doable. > > > > Yes, that is another miss-conception from the early days. The > > gpio-controllers themself are actually Synopsis Designware IPs - the > > kernel > > now even has a separate driver for them (gpio-dwapb). Only the real > > pincontrol/muxing/pull/etc is from Rockchip. > > > > Currently I can't think of a way to move over gracefully, without > > introducing crap into the gpio-dwapb driver. As at the moment it parses > > all > > information it needs from the dt directly - of course with different > > bindings. > > > > That is also the reason I do not want to introduce more "special-cases" in > > the bank-declaration we're using currently - to not make this move more > > difficult in the future. > > > > As it is, with the patch attached to the last mail, the pinctrl driver > > wouldn't even need the "rockchip,rk3188-gpio-bank0" compatible anymore, as > > the information about its special case is now sitting in the bank > > declaration inside the driver. Which then would enable us to remove the > > gpio-bank subnodes alltogether and use the external gpio driver. > > Okay, you convinced me. That sounds like a good plan to me. Maybe we can > introduce some compability code into the pinctrl driver to generate the > correct dt nodes for gpio-dwabp at runtime? I think that would depend on > something like CONFIG_OF_DYNAMIC though. Yep, that's one option. The other would be to let the driver support both (the internal gpio code and the real gpio-dwapb), deprecate the old one [of course also disallowing it for rk3288 and following] and simply remove it after the appropriate timespan (1 or more years if I remember correctly :-) ]. > > While for example syscon cannot handle clocks currently, the underlying > > regmap can - so it would be only a matter of teaching syscon to tell > > regmap > > of the clock to use (GRF and PMU register-clocks in this case), instead of > > needing to have every user of parts of these registers handle the relevant > > clock itself on register accesses. > > > > Also, as you can see in the grf map, the rk3188 actually has two > > soc_status > > registers (0xac and 0x108) which have the dmac, cpu and drive strength > > registers in between. So having a syscon only for soc_con and soc_status > > will produce problems too. > > > > Another example, GRF_IO_VSEL at 0x0380 is most likely some sort of pin > > voltage selection. As it only spans 1 register I'd assume it could contain > > settings that span all pin banks. > > Okay, we couldn't handle that with individual devices for each bank. > > > And of course, splitting the register space into dozens of small > > individual > > mappings looks messy :-) > > Yes, I agree now ;-) cool :-D I've also just sent a v2, including the missing MFD_SYSCON dependency, thanks for the find. Heiko
From 413669fdf083e5570fc544c08d998876d77b5b55 Mon Sep 17 00:00:00 2001 From: Heiko Stuebner <heiko@sntech.de> Date: Thu, 1 May 2014 00:47:01 +0200 Subject: [PATCH] pinctrl: rockchip: WIP introduce flexible iomux handling and rk3288 support todo: split into two patches, one introducing the flexible iomux handling and another one adding the actual rk3288 support. --- drivers/pinctrl/pinctrl-rockchip.c | 230 ++++++++++++++++++++++++++++++++----- 1 file changed, 202 insertions(+), 28 deletions(-) diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c index 948a19f..15f2d30 100644 --- a/drivers/pinctrl/pinctrl-rockchip.c +++ b/drivers/pinctrl/pinctrl-rockchip.c @@ -70,6 +70,29 @@ enum rockchip_pin_bank_type { }; /** + * Encode variants of iomux registers into a type variable + * bit[0] - iomux is gpio-only + * bit[3:1] - bit width of pin iomux settings + * bit[6:4] - location of this iomux register + */ +#define IOMUX_GPIO_ONLY (1 << 0) +#define IOMUX_WIDTH_2BIT 0 +#define IOMUX_WIDTH_4BIT (1 << 1) +#define IOMUX_SOURCE_GRF 0 +#define IOMUX_SOURCE_PMU (1 << 4) + +/** + * @type: iomux variant using IOMUX_* constants + * @offset: if initialized to -1 it will be autocalculated, by specifying + * an initial offset value the relevant source offset can be reset + * to a new value for autocalculating the following iomux registers. + */ +struct rockchip_iomux { + int type; + int offset; +}; + +/** * @reg_base: register base of the gpio bank * @reg_pull: optional separate register for additional pull settings * @clk: clock of the gpio bank @@ -78,6 +101,7 @@ enum rockchip_pin_bank_type { * @nr_pins: number of pins in this bank * @name: name of the bank * @bank_num: number of the bank, to account for holes + * @iomux: array describing the 4 iomux sources of the bank * @valid: are all necessary informations present * @of_node: dt node of this bank * @drvdata: common pinctrl basedata @@ -96,6 +120,7 @@ struct rockchip_pin_bank { char *name; u8 bank_num; enum rockchip_pin_bank_type bank_type; + struct rockchip_iomux iomux[4]; bool valid; struct device_node *of_node; struct rockchip_pinctrl *drvdata; @@ -111,6 +136,25 @@ struct rockchip_pin_bank { .bank_num = id, \ .nr_pins = pins, \ .name = label, \ + .iomux = { \ + { .offset = -1 }, \ + { .offset = -1 }, \ + { .offset = -1 }, \ + { .offset = -1 }, \ + }, \ + } + +#define PIN_BANK_IOMUX_FLAGS(id, pins, label, iom0, iom1, iom2, iom3) \ + { \ + .bank_num = id, \ + .nr_pins = pins, \ + .name = label, \ + .iomux = { \ + { .type = iom0, .offset = -1 }, \ + { .type = iom1, .offset = -1 }, \ + { .type = iom2, .offset = -1 }, \ + { .type = iom3, .offset = -1 }, \ + }, \ } /** @@ -121,7 +165,8 @@ struct rockchip_pin_ctrl { u32 nr_pins; char *label; enum rockchip_pinctrl_type type; - int mux_offset; + int grf_mux_offset; + int pmu_mux_offset; void (*pull_calc_reg)(struct rockchip_pin_bank *bank, int pin_num, struct regmap **regmap, int *reg, u8 *bit); @@ -343,24 +388,34 @@ static const struct pinctrl_ops rockchip_pctrl_ops = { static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin) { struct rockchip_pinctrl *info = bank->drvdata; + int iomux_num = (pin / 8); + struct regmap *regmap; unsigned int val; - int reg, ret; + int reg, ret, mask; u8 bit; - if (bank->bank_type == RK3188_BANK0 && pin < 16) + if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) return RK_FUNC_GPIO; + regmap = (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU) + ? info->regmap_pmu : info->regmap_base; + /* get basic quadrupel of mux registers and the correct reg inside */ - reg = info->ctrl->mux_offset; - reg += bank->bank_num * 0x10; - reg += (pin / 8) * 4; - bit = (pin % 8) * 2; + mask = (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) ? 0xf : 0x3; + reg = bank->iomux[iomux_num].offset; + if (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) { + if ((pin % 8) >= 4) + reg += 0x4; + bit = (pin % 4) * 4; + } else { + bit = (pin % 8) * 2; + } - ret = regmap_read(info->regmap_base, reg, &val); + ret = regmap_read(regmap, reg, &val); if (ret) return ret; - return ((val >> bit) & 3); + return ((val >> bit) & mask); } /* @@ -379,16 +434,14 @@ static int rockchip_get_mux(struct rockchip_pin_bank *bank, int pin) static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) { struct rockchip_pinctrl *info = bank->drvdata; - int reg, ret; + int iomux_num = (pin / 8); + struct regmap *regmap; + int reg, ret, mask; unsigned long flags; u8 bit; u32 data; - /* - * The first 16 pins of rk3188_bank0 are always gpios and do not have - * a mux register at all. - */ - if (bank->bank_type == RK3188_BANK0 && pin < 16) { + if (bank->iomux[iomux_num].type & IOMUX_GPIO_ONLY) { if (mux != RK_FUNC_GPIO) { dev_err(info->dev, "pin %d only supports a gpio mux\n", pin); @@ -401,17 +454,25 @@ static int rockchip_set_mux(struct rockchip_pin_bank *bank, int pin, int mux) dev_dbg(info->dev, "setting mux of GPIO%d-%d to %d\n", bank->bank_num, pin, mux); + regmap = (bank->iomux[iomux_num].type & IOMUX_SOURCE_PMU) + ? info->regmap_pmu : info->regmap_base; + /* get basic quadrupel of mux registers and the correct reg inside */ - reg = info->ctrl->mux_offset; - reg += bank->bank_num * 0x10; - reg += (pin / 8) * 4; - bit = (pin % 8) * 2; + mask = (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) ? 0xf : 0x3; + reg = bank->iomux[iomux_num].offset; + if (bank->iomux[iomux_num].type & IOMUX_WIDTH_4BIT) { + if ((pin % 8) >= 4) + reg += 0x4; + bit = (pin % 4) * 4; + } else { + bit = (pin % 8) * 2; + } spin_lock_irqsave(&bank->slock, flags); - data = (3 << (bit + 16)); - data |= (mux & 3) << bit; - ret = regmap_write(info->regmap_base, reg, data); + data = (mask << (bit + 16)); + data |= (mux & mask) << bit; + ret = regmap_write(regmap, reg, data); spin_unlock_irqrestore(&bank->slock, flags); @@ -475,6 +536,38 @@ static void rk3188_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank, } } +//FIXME: what is the real first pull registers +//FIXME: GPIO0A_PULL would be at 0x130 of the GRF, if it weren't in the pmu +//GPIO0A_PULL - GPIO0C_PULL are in the pmu instead +#define RK3288_PULL_OFFSET 0x140 +static void rk3288_calc_pull_reg_and_bit(struct rockchip_pin_bank *bank, + int pin_num, struct regmap **regmap, + int *reg, u8 *bit) +{ + struct rockchip_pinctrl *info = bank->drvdata; + + /* The first 24 pins of the first bank are located elsewhere */ + if (bank->bank_num == 0 && pin_num < 24) { + *regmap = info->regmap_pmu; + *reg = RK3188_PULL_PMU_OFFSET; + + *reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4); + *bit = pin_num % RK3188_PULL_PINS_PER_REG; + *bit *= RK3188_PULL_BITS_PER_PIN; + } else { + *regmap = info->regmap_base; + *reg = RK3288_PULL_OFFSET; + + /* correct the offset, as it is the 4th pull register */ + *reg -= 0x10; + *reg += bank->bank_num * RK3188_PULL_BANK_STRIDE; + *reg += ((pin_num / RK3188_PULL_PINS_PER_REG) * 4); + + *bit = (pin_num % RK3188_PULL_PINS_PER_REG); + *bit *= RK3188_PULL_BITS_PER_PIN; + } +} + static int rockchip_get_pull(struct rockchip_pin_bank *bank, int pin_num) { struct rockchip_pinctrl *info = bank->drvdata; @@ -1510,7 +1603,7 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data( struct device_node *np; struct rockchip_pin_ctrl *ctrl; struct rockchip_pin_bank *bank; - int i; + int grf_offs, pmu_offs, i, j; match = of_match_node(rockchip_pinctrl_dt_match, node); ctrl = (struct rockchip_pin_ctrl *)match->data; @@ -1532,12 +1625,45 @@ static struct rockchip_pin_ctrl *rockchip_pinctrl_get_soc_data( } } + grf_offs = ctrl->grf_mux_offset; + pmu_offs = ctrl->pmu_mux_offset; bank = ctrl->pin_banks; for (i = 0; i < ctrl->nr_banks; ++i, ++bank) { spin_lock_init(&bank->slock); bank->drvdata = d; bank->pin_base = ctrl->nr_pins; ctrl->nr_pins += bank->nr_pins; + + /* calculate iomux offsets */ + for (j = 0; j < 4; j++) { + struct rockchip_iomux *iom = &bank->iomux[j]; + int inc; + + /* preset offset value, set new start value */ + if (iom->offset >= 0) { + if (iom->type & IOMUX_SOURCE_PMU) + pmu_offs = iom->offset; + else + grf_offs = iom->offset; + } else { /* set current offset */ + iom->offset = (iom->type & IOMUX_SOURCE_PMU) ? + pmu_offs : grf_offs; + } + + dev_info(d->dev, "bank %d, iomux %d has offset 0x%x\n", + i, j, iom->offset); + + /* + * Increase offset according to iomux width. + * 4bit iomux'es are spread over two registers. + */ + inc = (iom->type & IOMUX_WIDTH_4BIT) ? 8 : 4; + if (iom->type & IOMUX_SOURCE_PMU) + pmu_offs += inc; + else + grf_offs += inc; + } + } return ctrl; @@ -1641,7 +1767,7 @@ static struct rockchip_pin_ctrl rk2928_pin_ctrl = { .nr_banks = ARRAY_SIZE(rk2928_pin_banks), .label = "RK2928-GPIO", .type = RK2928, - .mux_offset = 0xa8, + .grf_mux_offset = 0xa8, .pull_calc_reg = rk2928_calc_pull_reg_and_bit, }; @@ -1659,7 +1785,7 @@ static struct rockchip_pin_ctrl rk3066a_pin_ctrl = { .nr_banks = ARRAY_SIZE(rk3066a_pin_banks), .label = "RK3066a-GPIO", .type = RK2928, - .mux_offset = 0xa8, + .grf_mux_offset = 0xa8, .pull_calc_reg = rk2928_calc_pull_reg_and_bit, }; @@ -1675,11 +1801,11 @@ static struct rockchip_pin_ctrl rk3066b_pin_ctrl = { .nr_banks = ARRAY_SIZE(rk3066b_pin_banks), .label = "RK3066b-GPIO", .type = RK3066B, - .mux_offset = 0x60, + .grf_mux_offset = 0x60, }; static struct rockchip_pin_bank rk3188_pin_banks[] = { - PIN_BANK(0, 32, "gpio0"), + PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_GPIO_ONLY, 0, 0, 0), PIN_BANK(1, 32, "gpio1"), PIN_BANK(2, 32, "gpio2"), PIN_BANK(3, 32, "gpio3"), @@ -1690,10 +1816,56 @@ static struct rockchip_pin_ctrl rk3188_pin_ctrl = { .nr_banks = ARRAY_SIZE(rk3188_pin_banks), .label = "RK3188-GPIO", .type = RK3188, - .mux_offset = 0x60, + .grf_mux_offset = 0x60, .pull_calc_reg = rk3188_calc_pull_reg_and_bit, }; +static struct rockchip_pin_bank rk3288_pin_banks[] = { + PIN_BANK_IOMUX_FLAGS(0, 32, "gpio0", IOMUX_SOURCE_PMU, + IOMUX_SOURCE_PMU, + IOMUX_SOURCE_PMU, + IOMUX_SOURCE_PMU | IOMUX_GPIO_ONLY + ), + PIN_BANK_IOMUX_FLAGS(1, 32, "gpio1", IOMUX_GPIO_ONLY, + IOMUX_GPIO_ONLY, + IOMUX_GPIO_ONLY, + 0 + ), + PIN_BANK_IOMUX_FLAGS(2, 32, "gpio2", 0, 0, 0, IOMUX_GPIO_ONLY), + PIN_BANK_IOMUX_FLAGS(3, 32, "gpio3", 0, 0, 0, IOMUX_WIDTH_4BIT), + PIN_BANK_IOMUX_FLAGS(4, 32, "gpio4", IOMUX_WIDTH_4BIT, + IOMUX_WIDTH_4BIT, + 0, + 0 + ), + PIN_BANK_IOMUX_FLAGS(5, 32, "gpio5", IOMUX_GPIO_ONLY, + 0, + 0, + IOMUX_GPIO_ONLY + ), + PIN_BANK_IOMUX_FLAGS(6, 32, "gpio6", 0, 0, 0, IOMUX_GPIO_ONLY), + PIN_BANK_IOMUX_FLAGS(7, 32, "gpio7", 0, + 0, + IOMUX_WIDTH_4BIT, + IOMUX_GPIO_ONLY + ), + PIN_BANK_IOMUX_FLAGS(8, 32, "gpio8", 0, + 0, + IOMUX_GPIO_ONLY, + IOMUX_GPIO_ONLY + ), +}; + +static struct rockchip_pin_ctrl rk3288_pin_ctrl = { + .pin_banks = rk3288_pin_banks, + .nr_banks = ARRAY_SIZE(rk3288_pin_banks), + .label = "RK3288-GPIO", + .type = RK3188, + .grf_mux_offset = 0x0, + .pmu_mux_offset = 0x84, + .pull_calc_reg = rk3288_calc_pull_reg_and_bit, +}; + static const struct of_device_id rockchip_pinctrl_dt_match[] = { { .compatible = "rockchip,rk2928-pinctrl", .data = (void *)&rk2928_pin_ctrl }, @@ -1703,6 +1875,8 @@ static const struct of_device_id rockchip_pinctrl_dt_match[] = { .data = (void *)&rk3066b_pin_ctrl }, { .compatible = "rockchip,rk3188-pinctrl", .data = (void *)&rk3188_pin_ctrl }, + { .compatible = "rockchip,rk3288-pinctrl", + .data = (void *)&rk3288_pin_ctrl }, {}, }; MODULE_DEVICE_TABLE(of, rockchip_pinctrl_dt_match); -- 1.9.0