| Message ID | a266046d34009e6e92c4c76699c550c2ba44bd5c.1474986045.git-series.andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > The System Control Unit IP in the Aspeed SoCs is typically where the > pinmux configuration is found. > > But not always. > > On the AST2400 and AST2500 a number of pins depend on state in one of > the SIO, LPC or GFX IP blocks, so add support to at least capture what > that state is. The pinctrl engine for the Aspeed SoCs doesn't try to > inspect or modify the state of the off-SCU IP blocks. Instead, it logs > the state requirement with the expectation that the platform > designer/maintainer arranges for the appropriate configuration to be > applied through the associated drivers. This is unfortunate. This patch kicks the can down the road, but doesn't solve the problem for a user who wants to configure some functionality that depends on the non-SCU bits. Because of this I'm not sure if we want to put it in the tree. However, I'm not sure what a proper solution would look like. Perhaps Linus can point out another SoC that has a similar problem? Cheers, Joel > > The IP block of interest is encoded in the reg member of struct > aspeed_sig_desc. For compatibility with the existing code, the SCU is > defined to have an IP value of 0. > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/pinctrl/aspeed/pinctrl-aspeed.c | 53 +++++++++++++++++++++++--- > drivers/pinctrl/aspeed/pinctrl-aspeed.h | 16 +++++++- > 2 files changed, 61 insertions(+), 8 deletions(-) > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > index 49aeba912531..21ef195d586f 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c > @@ -14,6 +14,8 @@ > #include "../core.h" > #include "pinctrl-aspeed.h" > > +const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" }; > + > int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev) > { > struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev); > @@ -78,7 +80,9 @@ int aspeed_pinmux_get_fn_groups(struct pinctrl_dev *pctldev, > static inline void aspeed_sig_desc_print_val( > const struct aspeed_sig_desc *desc, bool enable, u32 rv) > { > - pr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg, > + pr_debug("Want %s%lX[0x%08X]=0x%X, got 0x%X from 0x%08X\n", > + aspeed_pinmux_ips[SIG_DESC_IP_FROM_REG(desc->reg)], > + SIG_DESC_OFFSET_FROM_REG(desc->reg), > desc->mask, enable ? desc->enable : desc->disable, > (rv & desc->mask) >> __ffs(desc->mask), rv); > } > @@ -105,6 +109,8 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc, > unsigned int raw; > u32 want; > > + WARN_ON(SIG_DESC_IP_FROM_REG(desc->reg) != ASPEED_IP_SCU); > + > if (regmap_read(map, desc->reg, &raw) < 0) > return false; > > @@ -142,9 +148,19 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr, > > for (i = 0; i < expr->ndescs; i++) { > const struct aspeed_sig_desc *desc = &expr->descs[i]; > + size_t ip = SIG_DESC_IP_FROM_REG(desc->reg); > + > + if (ip == ASPEED_IP_SCU) { > + if (!aspeed_sig_desc_eval(desc, enabled, map)) > + return false; > + } else { > + size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg); > + const char *ip_name = aspeed_pinmux_ips[ip]; > + > + pr_debug("Ignoring configuration of field %s%X[0x%08X]\n", > + ip_name, offset, desc->mask); > + } > > - if (!aspeed_sig_desc_eval(desc, enabled, map)) > - return false; > } > > return true; > @@ -170,7 +186,14 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, > for (i = 0; i < expr->ndescs; i++) { > bool ret; > const struct aspeed_sig_desc *desc = &expr->descs[i]; > + > + size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg); > + size_t ip = SIG_DESC_IP_FROM_REG(desc->reg); > + bool is_scu = (ip == ASPEED_IP_SCU); > + const char *ip_name = aspeed_pinmux_ips[ip]; > + > u32 pattern = enable ? desc->enable : desc->disable; > + u32 val = (pattern << __ffs(desc->mask)); > > /* > * Strap registers are configured in hardware or by early-boot > @@ -179,11 +202,27 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, > * deconfigured and is the reason we re-evaluate after writing > * all descriptor bits. > */ > - if (desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) > + if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2)) > continue; > > - ret = regmap_update_bits(map, desc->reg, desc->mask, > - pattern << __ffs(desc->mask)) == 0; > + /* > + * Sometimes we need help from IP outside the SCU to activate a > + * mux request. Report that we need its cooperation. > + */ > + if (enable && !is_scu) { > + pr_debug("Pinmux request for %s requires cooperation of %s IP: Need (%s%X[0x%08X] = 0x%08X\n", > + expr->function, ip_name, ip_name, offset, > + desc->mask, val); > + } > + > + /* And only read/write SCU registers */ > + if (!is_scu) { > + pr_debug("Skipping configuration of field %s%X[0x%08X]\n", > + ip_name, offset, desc->mask); > + continue; > + } > + > + ret = regmap_update_bits(map, desc->reg, desc->mask, val) == 0; > > if (!ret) > return ret; > @@ -343,6 +382,8 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function, > const struct aspeed_sig_expr **funcs; > const struct aspeed_sig_expr ***prios; > > + pr_debug("Muxing pin %d for %s\n", pin, pfunc->name); > + > if (!pdesc) > return -EINVAL; > > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h > index 3e72ef8c54bf..4384407d77fb 100644 > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h > @@ -232,6 +232,15 @@ > * group. > */ > > +#define ASPEED_IP_SCU 0 > +#define ASPEED_IP_SIO 1 > +#define ASPEED_IP_GFX 2 > +#define ASPEED_IP_LPC 3 > + > +#define SIG_DESC_TO_REG(ip, offset) (((ip) << 24) | (offset)) > +#define SIG_DESC_IP_FROM_REG(reg) (((reg) >> 24) & GENMASK(7, 0)) > +#define SIG_DESC_OFFSET_FROM_REG(reg) ((reg) & GENMASK(11, 0)) > + > /* > * The "Multi-function Pins Mapping and Control" table in the SoC datasheet > * references registers by the device/offset mnemonic. The register macros > @@ -261,7 +270,10 @@ > * A signal descriptor, which describes the register, bits and the > * enable/disable values that should be compared or written. > * > - * @reg: The register offset from base in bytes > + * @reg: Split into three fields: > + * 31:24: IP selector > + * 23:12: Reserved > + * 11:0: Register offset > * @mask: The mask to apply to the register. The lowest set bit of the mask is > * used to derive the shift value. > * @enable: The value that enables the function. Value should be in the LSBs, > @@ -270,7 +282,7 @@ > * LSBs, not at the position of the mask. > */ > struct aspeed_sig_desc { > - unsigned int reg; > + u32 reg; > u32 mask; > u32 enable; > u32 disable; > -- > git-series 0.8.10
On Thu, 2016-09-29 at 16:15 +0930, Joel Stanley wrote: > On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > The System Control Unit IP in the Aspeed SoCs is typically where the > > pinmux configuration is found. > > > > But not always. > > > > On the AST2400 and AST2500 a number of pins depend on state in one of > > the SIO, LPC or GFX IP blocks, so add support to at least capture what > > that state is. The pinctrl engine for the Aspeed SoCs doesn't try to > > inspect or modify the state of the off-SCU IP blocks. Instead, it logs > > the state requirement with the expectation that the platform > > designer/maintainer arranges for the appropriate configuration to be > > applied through the associated drivers. > This is unfortunate. > > This patch kicks the can down the road, but doesn't solve the problem > for a user who wants to configure some functionality that depends on > the non-SCU bits. Because of this I'm not sure if we want to put it in > the tree. I agree that there's not much functionality from a user's perspective, but the "kicking the can down the road" assessment might be a little harsh. Given the lack of user functionality it becomes more difficult to argue for the patch's inclusion given the additional complexity, but it does mean that the g4/g5 drivers can completely specify their dependencies and not have the aspeed pinctrl core do the wrong thing when it encounters the non-SCU IP offsets. It gets us half-way to having the pinctrl driver actually configure the state (knowing what it needs to configure), which I feel is more than a kick-the-can-down-the- road boondoggle. > > However, I'm not sure what a proper solution would look like. So if we accept that a proper solution includes specifying the off-SCU dependencies, the remaining question is how do we tastefully apply the desired state on register-spaces the pinctrl driver doesn't own. > Perhaps > Linus can point out another SoC that has a similar problem? Or failing that, an approach that is acceptable... Cheers, Andrew
On Thu, Sep 29, 2016 at 8:45 AM, Joel Stanley <joel@jms.id.au> wrote: > On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote: >> On the AST2400 and AST2500 a number of pins depend on state in one of >> the SIO, LPC or GFX IP blocks, so add support to at least capture what >> that state is. The pinctrl engine for the Aspeed SoCs doesn't try to >> inspect or modify the state of the off-SCU IP blocks. Instead, it logs >> the state requirement with the expectation that the platform >> designer/maintainer arranges for the appropriate configuration to be >> applied through the associated drivers. (...) > > This is unfortunate. > > This patch kicks the can down the road, but doesn't solve the problem > for a user who wants to configure some functionality that depends on > the non-SCU bits. Because of this I'm not sure if we want to put it in > the tree. > > However, I'm not sure what a proper solution would look like. Perhaps > Linus can point out another SoC that has a similar problem? If I could only understand it. Don't you actually want to go and read a few registers on another IO range? What we generally do when pin control is spread out in a system is try to consolidate it, meaning expose the necessary registers on the remote end using syscon (drivers/mfd/syscon) so that we can easily get a handle on these registers withe regmap MMIO. Since regmap handles atomic access to the registers, that way we protect from disasters and keep the state in the hardware. I don't know if this is helpful. For a normal peripheral you may not want to put a regmap over all its registers but you prefer to ioremap it, and then we get the spaghetti of accessor functions to peek and poke into another peripherals I/O space, and that is undesireable. Maybe this is completely not understanding what you want to do, so sorry, please elaborate. I am afraid the two of you are the only people on the planet who actually understand what is going on here. Also the hardware engineer who wrote the Aspeed pin controller, I would like to read this persons design specification, I am pretty sure it is very interesting. >> - * @reg: The register offset from base in bytes >> + * @reg: Split into three fields: >> + * 31:24: IP selector >> + * 23:12: Reserved >> + * 11:0: Register offset That seems like unnecessary shoehorning. Is it reflecting the register layout of the hardware or are you trying to save bits? For the latter, let it go and instead have one member for offset and one member for selector. Yours, Linus Walleij
On Mon, 2016-10-24 at 00:20 +0200, Linus Walleij wrote: > On Thu, Sep 29, 2016 at 8:45 AM, Joel Stanley <joel@jms.id.au> wrote: > > > > On Wed, Sep 28, 2016 at 12:20 AM, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > > > > > On the AST2400 and AST2500 a number of pins depend on state in one of > > > the SIO, LPC or GFX IP blocks, so add support to at least capture what > > > that state is. The pinctrl engine for the Aspeed SoCs doesn't try to > > > inspect or modify the state of the off-SCU IP blocks. Instead, it logs > > > the state requirement with the expectation that the platform > > > designer/maintainer arranges for the appropriate configuration to be > > > applied through the associated drivers. > (...) > > > > > > This is unfortunate. > > > > This patch kicks the can down the road, but doesn't solve the problem > > for a user who wants to configure some functionality that depends on > > the non-SCU bits. Because of this I'm not sure if we want to put it in > > the tree. > > > > However, I'm not sure what a proper solution would look like. Perhaps > > Linus can point out another SoC that has a similar problem? > If I could only understand it. > > Don't you actually want to go and read a few registers on another > IO range? Yes. I guess the hesitation was whether we should also write those registers so we can apply the requested function. > > What we generally do when pin control is spread out in a system is try > to consolidate it, meaning expose the necessary registers on the remote > end using syscon (drivers/mfd/syscon) so that we can easily get a handle > on these registers withe regmap MMIO. > > Since regmap handles atomic access to the registers, that way we > protect from disasters and keep the state in the hardware. This seems like the reasonable approach if we want to read/write those registers. The affected IO ranges correspond to: * SuperIO Controller (SIO) * SOC Display Controller (GFX) * LPC Controller (LPC) SIO and LPC both perform a mishmash of functions and so likely would use the mfd subsystem anyway. I don't yet have any arguments against doing it for the GFX IO space. Joel? > > I don't know if this is helpful. For a normal peripheral you may not want > to put a regmap over all its registers but you prefer to ioremap it, and then > we get the spaghetti of accessor functions to peek and poke into another > peripherals I/O space, and that is undesireable. I briefly experimented with the idea of accessing the other IO spaces but it felt dirty precisely for what would have become accessor spaghetti. So I wound up with the lame approach in this patch, which just punts on the problem. I think the mfd/syscon approach would work well though. It looks like the rockchip pinctrl bindings are doing something along the lines of what we need with the rockchip,pmu phandle property. Is it acceptable to create three properties, a phandle to grab each regmap for the IO spaces above? > > Maybe this is completely not understanding what you want to do, so > sorry, please elaborate. No, seems you have understood what we need to do. > I am afraid the two of you are the only people on > the planet who actually understand what is going on here. > > Also the hardware engineer who wrote the Aspeed pin controller, I would > like to read this persons design specification, I am pretty sure it is very > interesting. Well, presumably this engineer knows too :) And yes, I'd like to know what constraints existed that forced this design as a solution. I'd like my sanity back. > > > > > > > > > - * @reg: The register offset from base in bytes > > > + * @reg: Split into three fields: > > > + * 31:24: IP selector > > > + * 23:12: Reserved > > > + * 11:0: Register offset > That seems like unnecessary shoehorning. Is it reflecting the register layout > of the hardware or are you trying to save bits? For the latter, let it > go and instead > have one member for offset and one member for selector. It doesn't represent the register layout. But saving bits also wasn't the motivation, more avoiding a lot of code churn in the g4/g5 drivers to populate a new member. Maybe that's a bad motivation. I'll have more of a think about it. Thanks for the feedback, Andrew
diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c index 49aeba912531..21ef195d586f 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c @@ -14,6 +14,8 @@ #include "../core.h" #include "pinctrl-aspeed.h" +const char *const aspeed_pinmux_ips[] = { "SCU", "SIO", "GFX", "LPC" }; + int aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev) { struct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev); @@ -78,7 +80,9 @@ int aspeed_pinmux_get_fn_groups(struct pinctrl_dev *pctldev, static inline void aspeed_sig_desc_print_val( const struct aspeed_sig_desc *desc, bool enable, u32 rv) { - pr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg, + pr_debug("Want %s%lX[0x%08X]=0x%X, got 0x%X from 0x%08X\n", + aspeed_pinmux_ips[SIG_DESC_IP_FROM_REG(desc->reg)], + SIG_DESC_OFFSET_FROM_REG(desc->reg), desc->mask, enable ? desc->enable : desc->disable, (rv & desc->mask) >> __ffs(desc->mask), rv); } @@ -105,6 +109,8 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc, unsigned int raw; u32 want; + WARN_ON(SIG_DESC_IP_FROM_REG(desc->reg) != ASPEED_IP_SCU); + if (regmap_read(map, desc->reg, &raw) < 0) return false; @@ -142,9 +148,19 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr, for (i = 0; i < expr->ndescs; i++) { const struct aspeed_sig_desc *desc = &expr->descs[i]; + size_t ip = SIG_DESC_IP_FROM_REG(desc->reg); + + if (ip == ASPEED_IP_SCU) { + if (!aspeed_sig_desc_eval(desc, enabled, map)) + return false; + } else { + size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg); + const char *ip_name = aspeed_pinmux_ips[ip]; + + pr_debug("Ignoring configuration of field %s%X[0x%08X]\n", + ip_name, offset, desc->mask); + } - if (!aspeed_sig_desc_eval(desc, enabled, map)) - return false; } return true; @@ -170,7 +186,14 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, for (i = 0; i < expr->ndescs; i++) { bool ret; const struct aspeed_sig_desc *desc = &expr->descs[i]; + + size_t offset = SIG_DESC_OFFSET_FROM_REG(desc->reg); + size_t ip = SIG_DESC_IP_FROM_REG(desc->reg); + bool is_scu = (ip == ASPEED_IP_SCU); + const char *ip_name = aspeed_pinmux_ips[ip]; + u32 pattern = enable ? desc->enable : desc->disable; + u32 val = (pattern << __ffs(desc->mask)); /* * Strap registers are configured in hardware or by early-boot @@ -179,11 +202,27 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr, * deconfigured and is the reason we re-evaluate after writing * all descriptor bits. */ - if (desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) + if (is_scu && (offset == HW_STRAP1 || offset == HW_STRAP2)) continue; - ret = regmap_update_bits(map, desc->reg, desc->mask, - pattern << __ffs(desc->mask)) == 0; + /* + * Sometimes we need help from IP outside the SCU to activate a + * mux request. Report that we need its cooperation. + */ + if (enable && !is_scu) { + pr_debug("Pinmux request for %s requires cooperation of %s IP: Need (%s%X[0x%08X] = 0x%08X\n", + expr->function, ip_name, ip_name, offset, + desc->mask, val); + } + + /* And only read/write SCU registers */ + if (!is_scu) { + pr_debug("Skipping configuration of field %s%X[0x%08X]\n", + ip_name, offset, desc->mask); + continue; + } + + ret = regmap_update_bits(map, desc->reg, desc->mask, val) == 0; if (!ret) return ret; @@ -343,6 +382,8 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function, const struct aspeed_sig_expr **funcs; const struct aspeed_sig_expr ***prios; + pr_debug("Muxing pin %d for %s\n", pin, pfunc->name); + if (!pdesc) return -EINVAL; diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h index 3e72ef8c54bf..4384407d77fb 100644 --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h @@ -232,6 +232,15 @@ * group. */ +#define ASPEED_IP_SCU 0 +#define ASPEED_IP_SIO 1 +#define ASPEED_IP_GFX 2 +#define ASPEED_IP_LPC 3 + +#define SIG_DESC_TO_REG(ip, offset) (((ip) << 24) | (offset)) +#define SIG_DESC_IP_FROM_REG(reg) (((reg) >> 24) & GENMASK(7, 0)) +#define SIG_DESC_OFFSET_FROM_REG(reg) ((reg) & GENMASK(11, 0)) + /* * The "Multi-function Pins Mapping and Control" table in the SoC datasheet * references registers by the device/offset mnemonic. The register macros @@ -261,7 +270,10 @@ * A signal descriptor, which describes the register, bits and the * enable/disable values that should be compared or written. * - * @reg: The register offset from base in bytes + * @reg: Split into three fields: + * 31:24: IP selector + * 23:12: Reserved + * 11:0: Register offset * @mask: The mask to apply to the register. The lowest set bit of the mask is * used to derive the shift value. * @enable: The value that enables the function. Value should be in the LSBs, @@ -270,7 +282,7 @@ * LSBs, not at the position of the mask. */ struct aspeed_sig_desc { - unsigned int reg; + u32 reg; u32 mask; u32 enable; u32 disable;
The System Control Unit IP in the Aspeed SoCs is typically where the pinmux configuration is found. But not always. On the AST2400 and AST2500 a number of pins depend on state in one of the SIO, LPC or GFX IP blocks, so add support to at least capture what that state is. The pinctrl engine for the Aspeed SoCs doesn't try to inspect or modify the state of the off-SCU IP blocks. Instead, it logs the state requirement with the expectation that the platform designer/maintainer arranges for the appropriate configuration to be applied through the associated drivers. The IP block of interest is encoded in the reg member of struct aspeed_sig_desc. For compatibility with the existing code, the SCU is defined to have an IP value of 0. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/pinctrl/aspeed/pinctrl-aspeed.c | 53 +++++++++++++++++++++++--- drivers/pinctrl/aspeed/pinctrl-aspeed.h | 16 +++++++- 2 files changed, 61 insertions(+), 8 deletions(-)