Re: [PATCH v2 4/6] pinctrl: aspeed: Read and write bits in LPCHC and GFX controllers

From: Andrew Jeffery
Date: Fri Nov 04 2016 - 00:01:19 EST

Next message: Alex Williamson: "Re: [RFC 0/8] KVM PCIe/MSI passthrough on ARM/ARM64 (Alt II)"
Previous message: Andy Lutomirski: "Re: vmalloced stacks and scatterwalk_map_and_copy()"
In reply to: Joel Stanley: "Re: [PATCH v2 4/6] pinctrl: aspeed: Read and write bits in LPCHC and GFX controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 2016-11-04 at 09:54 +1030, Joel Stanley wrote:
> On Thu, Nov 3, 2016 at 1:07 AM, Andrew Jeffery <andrew@xxxxxxxx> wrote:
> >
> > The System Control Unit IP block in the Aspeed SoCs is typically where
> > the pinmux configuration is found, but not always. A number of pins
> > depend on state in one of LPC Host Control (LPCHC) or SoC Display
> > Controller (GFX) IP blocks, so the Aspeed pinmux drivers should have the
> > means to adjust these as necessary.
> >
> > We use syscon to cast a regmap over the GFX and LPCHCR blocks, which is
> > used as an arbitration layer between the relevant driver and the pinctrl
> > subsystem. The regmaps are then exposed to the SoC-specific pinctrl
> > drivers by phandles in the devicetree, and are selected during a mux
> > request by querying a new 'ip' member in struct aspeed_sig_desc.
> >
> > Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
> I like this a lot more than the first go. Good work.
>
> Some minor comments below.
>
> >
> > ---
> > Since v1:
> >
> > The change is now proactive: instead of reporting that we need to flip bits in
> > controllers we can't access, the patch provides access via regmaps for the
> > relevant controllers. The implementation also splits out the IP block ID into
> > its own variable rather than packing the value into the upper bits of the reg
> > member of struct aspeed_sig_desc. This drives some churn in the diff, but I've
> > tried to minimise it.
> >
> > Â.../devicetree/bindings/pinctrl/pinctrl-aspeed.txt | 50 +++++++++++++---
> > Âdrivers/pinctrl/aspeed/pinctrl-aspeed-g4.cÂÂÂÂÂÂÂÂÂ| 18 +++---
> > Âdrivers/pinctrl/aspeed/pinctrl-aspeed-g5.cÂÂÂÂÂÂÂÂÂ| 39 ++++++++++---
> > Âdrivers/pinctrl/aspeed/pinctrl-aspeed.cÂÂÂÂÂÂÂÂÂÂÂÂ| 66 +++++++++++++---------
> > Âdrivers/pinctrl/aspeed/pinctrl-aspeed.hÂÂÂÂÂÂÂÂÂÂÂÂ| 32 ++++++++---
> > Â5 files changed, 144 insertions(+), 61 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt
> > index 2ad18c4ea55c..115b0cce6c1c 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt
> > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt
> > @@ -4,12 +4,19 @@ Aspeed Pin Controllers
> > ÂThe Aspeed SoCs vary in functionality inside a generation but have a common mux
> > Âdevice register layout.
> >
> > -Required properties:
> > -- compatible : Should be any one of the following:
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,ast2400-pinctrl"
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,g4-pinctrl"
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,ast2500-pinctrl"
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,g5-pinctrl"
> > +Required properties for g4:
> > +- compatible :ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂShould be any one of the following:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,ast2400-pinctrl"
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,g4-pinctrl"
> > +
> > +Required properties for g5:
> > +- compatible :ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂShould be any one of the following:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,ast2500-pinctrl"
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ"aspeed,g5-pinctrl"
> > +
> > +- aspeed,external-nodes:ÂÂÂÂÂÂÂA cell of phandles to external controller nodes:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ0: compatible with "aspeed,ast2500-gfx", "syscon"
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ1: compatible with "aspeed,ast2500-lpchc", "syscon"
> >
> > ÂThe pin controller node should be a child of a syscon node with the required
> > Âproperty:
> > @@ -47,7 +54,7 @@ RGMII1 RGMII2 RMII1 RMII2 SD1 SPI1 SPI1DEBUG SPI1PASSTHRU TIMER4 TIMER5 TIMER6
> > ÂTIMER7 TIMER8 VGABIOSROM
> >
> >
> > -Examples:
> > +g4 Example:
> >
> > Âsyscon: scu@1e6e2000 {
> > ÂÂÂÂÂÂÂÂcompatible = "syscon", "simple-mfd";
> > @@ -63,5 +70,34 @@ syscon: scu@1e6e2000 {
> > ÂÂÂÂÂÂÂÂ};
> > Â};
> >
> > +g5 Example:
> > +
> > +apb {
> > +ÂÂÂÂÂÂÂgfx: display@1e6e6000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "aspeed,ast2500-gfx", "syscon";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x1e6e6000 0x1000>;
> > +ÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂlpchc: lpchc@1e7890a0 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "aspeed,ast2500-lpchc", "syscon";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x1e7890a0 0xc4>;
> > +ÂÂÂÂÂÂÂ};
> > +
> > +ÂÂÂÂÂÂÂsyscon: scu@1e6e2000 {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "syscon", "simple-mfd";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreg = <0x1e6e2000 0x1a8>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpinctrl: pinctrl {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcompatible = "aspeed,g5-pinctrl";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂaspeed,external-nodes = <&gfx, &lpchc>;
> > +
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpinctrl_i2c3_default: i2c3_default {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂfunction = "I2C3";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂgroups = "I2C3";
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ};
> > +ÂÂÂÂÂÂÂ};
> > +};
> > +
> > ÂPlease refer to pinctrl-bindings.txt in this directory for details of the
> > Âcommon pinctrl bindings used by client devices.
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > index a21b071ff290..558bd102416c 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g4.c
> > @@ -292,7 +292,7 @@ SSSF_PIN_DECL(U18, GPIOG7, FLWP, SIG_DESC_SET(SCU84, 7));
> > Â#define UART6_DESCÂÂÂÂÂSIG_DESC_SET(SCU90, 7)
> > Â#define ROM16_DESCÂÂÂÂÂSIG_DESC_SET(SCU90, 6)
> > Â#define FLASH_WIDEÂÂÂÂÂSIG_DESC_SET(HW_STRAP1, 4)
> > -#define BOOT_SRC_NORÂÂÂ{ HW_STRAP1, GENMASK(1, 0), 0, 0 }
> > +#define BOOT_SRC_NORÂÂÂ{ ASPEED_IP_SCU, HW_STRAP1, GENMASK(1, 0), 0, 0 }
> >
> > Â#define A8 56
> > ÂSIG_EXPR_DECL(ROMD8, ROM16, ROM16_DESC);
> > @@ -418,9 +418,9 @@ FUNC_GROUP_DECL(I2C8, G5, F3);
> > Â#define U1 88
> > ÂSSSF_PIN_DECL(U1, GPIOL0, NCTS1, SIG_DESC_SET(SCU84, 16));
> >
> > -#define VPI18_DESCÂÂÂÂÂ{ SCU90, GENMASK(5, 4), 1, 0 }
> > -#define VPI24_DESCÂÂÂÂÂ{ SCU90, GENMASK(5, 4), 2, 0 }
> > -#define VPI30_DESCÂÂÂÂÂ{ SCU90, GENMASK(5, 4), 3, 0 }
> > +#define VPI18_DESCÂÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 1, 0 }
> > +#define VPI24_DESCÂÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 2, 0 }
> > +#define VPI30_DESCÂÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 3, 0 }
> >
> > Â#define T5 89
> > Â#define T5_DESCÂÂÂÂÂÂÂÂÂSIG_DESC_SET(SCU84, 17)
> > @@ -641,11 +641,11 @@ SSSF_PIN_DECL(Y22, GPIOR2, ROMCS3, SIG_DESC_SET(SCU88, 26));
> > Â#define U19 139
> > ÂSSSF_PIN_DECL(U19, GPIOR3, ROMCS4, SIG_DESC_SET(SCU88, 27));
> >
> > -#define VPOOFF0_DESCÂÂÂ{ SCU94, GENMASK(1, 0), 0, 0 }
> > -#define VPO12_DESCÂÂÂÂÂ{ SCU94, GENMASK(1, 0), 1, 0 }
> > -#define VPO24_DESCÂÂÂÂÂ{ SCU94, GENMASK(1, 0), 2, 0 }
> > -#define VPOOFF1_DESCÂÂÂ{ SCU94, GENMASK(1, 0), 3, 0 }
> > -#define VPO_OFF_12ÂÂÂÂÂÂ{ SCU94, 0x2, 0, 0 }
> > +#define VPOOFF0_DESCÂÂÂ{ ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 }
> > +#define VPO12_DESCÂÂÂÂÂ{ ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 1, 0 }
> > +#define VPO24_DESCÂÂÂÂÂ{ ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 2, 0 }
> > +#define VPOOFF1_DESCÂÂÂ{ ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 3, 0 }
> > +#define VPO_OFF_12ÂÂÂÂÂÂ{ ASPEED_IP_SCU, SCU94, 0x2, 0, 0 }
> > Â#define VPO_24_OFFÂÂÂÂÂÂSIG_DESC_SET(SCU94, 1)
> >
> > Â#define V21 140
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
> > index 87b46390b695..99c4fa9bf861 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c
> > @@ -10,6 +10,7 @@
> > Â#include
> > Â#include
> > Â#include
> > +#include
> > Â#include
> > Â#include
> > Â#include
> > @@ -26,8 +27,8 @@
> >
> > Â#define ASPEED_G5_NR_PINS 228
> >
> > -#define COND1ÂÂÂÂÂÂÂÂÂÂ{ SCU90, BIT(6), 0, 0 }
> > -#define COND2ÂÂÂÂÂÂÂÂÂÂ{ SCU94, GENMASK(1, 0), 0, 0 }
> > +#define COND1ÂÂÂÂÂÂÂÂÂÂ{ ASPEED_IP_SCU, SCU90, BIT(6), 0, 0 }
> > +#define COND2ÂÂÂÂÂÂÂÂÂÂ{ ASPEED_IP_SCU, SCU94, GENMASK(1, 0), 0, 0 }
> >
> > Â#define B14 0
> > ÂSSSF_PIN_DECL(B14, GPIOA0, MAC1LINK, SIG_DESC_SET(SCU80, 0));
> > @@ -186,9 +187,12 @@ MS_PIN_DECL(C20, GPIOE1, NDCD3, GPIE0OUT);
> >
> > ÂFUNC_GROUP_DECL(GPIE0, B20, C20);
> >
> > -#define SPI1_DESCÂÂÂÂÂÂÂÂÂÂÂÂÂÂ{ HW_STRAP1, GENMASK(13, 12), 1, 0 }
> > -#define SPI1DEBUG_DESCÂÂÂÂÂÂÂÂÂ{ HW_STRAP1, GENMASK(13, 12), 2, 0 }
> > -#define SPI1PASSTHRU_DESCÂÂÂÂÂÂ{ HW_STRAP1, GENMASK(13, 12), 3, 0 }
> > +#define SPI1_DESC \
> > +ÂÂÂÂÂÂÂ{ ASPEED_IP_SCU, HW_STRAP1, GENMASK(13, 12), 1, 0 }
> > +#define SPI1DEBUG_DESC \
> > +ÂÂÂÂÂÂÂ{ ASPEED_IP_SCU, HW_STRAP1, GENMASK(13, 12), 2, 0 }
> > +#define SPI1PASSTHRU_DESC \
> > +ÂÂÂÂÂÂÂ{ ASPEED_IP_SCU, HW_STRAP1, GENMASK(13, 12), 3, 0 }
> >
> > Â#define C18 64
> > ÂSIG_EXPR_DECL(SYSCS, SPI1DEBUG, COND1, SPI1DEBUG_DESC);
> > @@ -325,10 +329,11 @@ SS_PIN_DECL(R1, GPIOK7, SDA8);
> >
> > ÂFUNC_GROUP_DECL(I2C8, P2, R1);
> >
> > -#define VPIOFF0_DESCÂÂÂÂ{ SCU90, GENMASK(5, 4), 0, 0 }
> > -#define VPIOFF1_DESCÂÂÂÂ{ SCU90, GENMASK(5, 4), 1, 0 }
> > -#define VPI24_DESCÂÂÂÂÂÂ{ SCU90, GENMASK(5, 4), 2, 0 }
> > -#define VPIRSVD_DESCÂÂÂÂ{ SCU90, GENMASK(5, 4), 3, 0 }
> > +#define VPIOFF0_DESCÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 0, 0 }
> > +#define VPIOFF1_DESCÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 1, 0 }
> > +#define VPI24_DESCÂÂÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 2, 0 }
> > +#define VPIRSVD_DESCÂÂÂÂ{ ASPEED_IP_SCU, SCU90, GENMASK(5, 4), 3, 0 }
> > +
> >
> > Â#define V2 104
> > Â#define V2_DESCÂÂÂÂÂÂÂÂÂSIG_DESC_SET(SCU88, 0)
> > @@ -848,10 +853,26 @@ static struct pinctrl_desc aspeed_g5_pinctrl_desc = {
> > Âstatic int aspeed_g5_pinctrl_probe(struct platform_device *pdev)
> > Â{
> > ÂÂÂÂÂÂÂÂint i;
> > +ÂÂÂÂÂÂÂstruct regmap **map;
> > +ÂÂÂÂÂÂÂstruct device_node *node;
> >
> > ÂÂÂÂÂÂÂÂfor (i = 0; i < ARRAY_SIZE(aspeed_g5_pins); i++)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂaspeed_g5_pins[i].number = i;
> >
> > +ÂÂÂÂÂÂÂmap = &aspeed_g5_pinctrl_data.maps[ASPEED_IP_GFX];
> > +ÂÂÂÂÂÂÂnode = of_parse_phandle(pdev->dev.of_node, "aspeed,external-nodes", 0);
> > +ÂÂÂÂÂÂÂ*map = syscon_node_to_regmap(node);
> I think you can use syscon_regmap_lookup_by_phandle to replace both of
> these lines.

Good call, will do.

>
> >
> > +ÂÂÂÂÂÂÂof_node_put(node);
> > +ÂÂÂÂÂÂÂif (IS_ERR(*map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(*map);
> Do we want to fail, or warn and continue?

We would need to add further checks to defend against null dereferences
if we were to continue. I think the broken devicetree should be fixed.

>
> The sequence is a bit messy. How about:
>
> struct regmap *map;
>
> map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> "aspeed,external-nodes");
> if (IS_ERR(map))
> ÂÂÂreturn PTR_ERR(map);
>
> aspeed_g5_pinctrl_data.maps[ASPEED_IP_GFX] = map;

That looks neater, I will switch.

>
>
> >
> > +
> > +ÂÂÂÂÂÂÂmap = &aspeed_g5_pinctrl_data.maps[ASPEED_IP_LPCHC];
> > +ÂÂÂÂÂÂÂnode = of_parse_phandle(pdev->dev.of_node, "aspeed,external-nodes", 1);
> > +ÂÂÂÂÂÂÂ*map = syscon_node_to_regmap(node);
> > +ÂÂÂÂÂÂÂof_node_put(node);
> > +ÂÂÂÂÂÂÂif (IS_ERR(*map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(*map);
> > +
> Same comments as above.

Ack.

>
> >
> > ÂÂÂÂÂÂÂÂreturn aspeed_pinctrl_probe(pdev, &aspeed_g5_pinctrl_desc,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ&aspeed_g5_pinctrl_data);
> > Â}
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > index 49aeba912531..23586aac7a5a 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> > @@ -14,6 +14,12 @@
> > Â#include "../core.h"
> > Â#include "pinctrl-aspeed.h"
> >
> > +static const char *const aspeed_pinmux_ips[] = {
> > +ÂÂÂÂÂÂÂ[ASPEED_IP_SCU] = "SCU",
> > +ÂÂÂÂÂÂÂ[ASPEED_IP_GFX] = "GFX",
> > +ÂÂÂÂÂÂÂ[ASPEED_IP_LPCHC] = "LHCR",
> We've got both LPCHC and LHCR here. As I said when commenting on the
> regmap bindings, I like LHC(R) better.
>
> >
> > +};
> > +
> > Âint aspeed_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> > Â{
> > ÂÂÂÂÂÂÂÂstruct aspeed_pinctrl_data *pdata = pinctrl_dev_get_drvdata(pctldev);
> > @@ -78,7 +84,8 @@ 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)
> What's a rv? Perhaps "reg" or "value"?
>
> >
> > Â{
> > -ÂÂÂÂÂÂÂpr_debug("SCU%x[0x%08x]=0x%x, got 0x%x from 0x%08x\n", desc->reg,
> > +ÂÂÂÂÂÂÂpr_debug("Want %s%X[0x%08X]=0x%X, got 0x%X from 0x%08X\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂaspeed_pinmux_ips[desc->ip], desc->reg,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdesc->mask, enable ? desc->enable : desc->disable,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ(rv & desc->mask) >> __ffs(desc->mask), rv);
> > Â}
> > @@ -88,7 +95,7 @@ static inline void aspeed_sig_desc_print_val(
> > Â *
> > Â * @desc: The signal descriptor of interest
> > Â * @enabled: True to query the enabled state, false to query disabled state
> > - * @regmap: The SCU regmap instance
> > + * @regmap: The IP block's regmap instance
> > Â *
> > Â * @return True if the descriptor's bitfield is configured to the state
> > Â * selected by @enabled, false otherwise
> > @@ -119,7 +126,7 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc,
> > Â *
> > Â * @expr: An expression controlling the signal for a mux function on a pin
> > Â * @enabled: True to query the enabled state, false to query disabled state
> > - * @regmap: The SCU regmap instance
> > + * @maps: The list of regmap instances
> > Â *
> > Â * @return True if the expression composed by @enabled evaluates true, false
> > Â * otherwise
> > @@ -136,15 +143,16 @@ static bool aspeed_sig_desc_eval(const struct aspeed_sig_desc *desc,
> > Â * either condition as required.
> > Â */
> > Âstatic bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool enabled, struct regmap *map)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool enabled, struct regmap * const *maps)
> > Â{
> > ÂÂÂÂÂÂÂÂint i;
> >
> > ÂÂÂÂÂÂÂÂfor (i = 0; i < expr->ndescs; i++) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst struct aspeed_sig_desc *desc = &expr->descs[i];
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_sig_desc_eval(desc, enabled, map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_sig_desc_eval(desc, enabled, maps[desc->ip]))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn false;
> > +
> > ÂÂÂÂÂÂÂÂ}
> >
> > ÂÂÂÂÂÂÂÂreturn true;
> > @@ -158,12 +166,12 @@ static bool aspeed_sig_expr_eval(const struct aspeed_sig_expr *expr,
> > Â *ÂÂÂÂÂÂÂÂconfigured
> > Â * @enable: true to enable an function's signal through a pin's signal
> > Â *ÂÂÂÂÂÂÂÂÂÂexpression, false to disable the function's signal
> > - * @map: The SCU's regmap instance for pinmux register access.
> > + * @maps: The list of regmap instances for pinmux register access.
> > Â *
> > Â * @return true if the expression is configured as requested, false otherwise
> > Â */
> > Âstatic bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool enable, struct regmap *map)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool enable, struct regmap * const *maps)
> > Â{
> > ÂÂÂÂÂÂÂÂint i;
> >
> > @@ -171,6 +179,7 @@ static bool aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool ret;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂconst struct aspeed_sig_desc *desc = &expr->descs[i];
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂu32 pattern = enable ? desc->enable : desc->disable;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂu32 val = (pattern << __ffs(desc->mask));
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/*
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ* Strap registers are configured in hardware or by early-boot
> > @@ -179,48 +188,49 @@ 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 ((desc->reg == HW_STRAP1 || desc->reg == HW_STRAP2) &&
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdesc->ip == ASPEED_IP_SCU)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcontinue;
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = regmap_update_bits(map, desc->reg, desc->mask,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂpattern << __ffs(desc->mask)) == 0;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = regmap_update_bits(maps[desc->ip], desc->reg,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdesc->mask, val) == 0;
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!ret)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > ÂÂÂÂÂÂÂÂ}
> >
> > -ÂÂÂÂÂÂÂreturn aspeed_sig_expr_eval(expr, enable, map);
> > +ÂÂÂÂÂÂÂreturn aspeed_sig_expr_eval(expr, enable, maps);
> > Â}
> >
> > Âstatic bool aspeed_sig_expr_enable(const struct aspeed_sig_expr *expr,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap *map)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap * const *maps)
> > Â{
> > -ÂÂÂÂÂÂÂif (aspeed_sig_expr_eval(expr, true, map))
> > +ÂÂÂÂÂÂÂif (aspeed_sig_expr_eval(expr, true, maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn true;
> >
> > -ÂÂÂÂÂÂÂreturn aspeed_sig_expr_set(expr, true, map);
> > +ÂÂÂÂÂÂÂreturn aspeed_sig_expr_set(expr, true, maps);
> > Â}
> >
> > Âstatic bool aspeed_sig_expr_disable(const struct aspeed_sig_expr *expr,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap *map)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap * const *maps)
> > Â{
> > -ÂÂÂÂÂÂÂif (!aspeed_sig_expr_eval(expr, true, map))
> > +ÂÂÂÂÂÂÂif (!aspeed_sig_expr_eval(expr, true, maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn true;
> >
> > -ÂÂÂÂÂÂÂreturn aspeed_sig_expr_set(expr, false, map);
> > +ÂÂÂÂÂÂÂreturn aspeed_sig_expr_set(expr, false, maps);
> > Â}
> >
> > Â/**
> > Â * Disable a signal on a pin by disabling all provided signal expressions.
> > Â *
> > Â * @exprs: The list of signal expressions (from a priority level on a pin)
> > - * @map: The SCU's regmap instance for pinmux register access.
> > + * @maps: The list of regmap instances for pinmux register access.
> > Â *
> > Â * @return true if all expressions in the list are successfully disabled, false
> > Â * otherwise
> > Â */
> > Âstatic bool aspeed_disable_sig(const struct aspeed_sig_expr **exprs,
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap *map)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂstruct regmap * const *maps)
> > Â{
> > ÂÂÂÂÂÂÂÂbool disabled = true;
> >
> > @@ -230,7 +240,7 @@ static bool aspeed_disable_sig(const struct aspeed_sig_expr **exprs,
> > ÂÂÂÂÂÂÂÂwhile (*exprs) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbool ret;
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = aspeed_sig_expr_disable(*exprs, map);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret = aspeed_sig_expr_disable(*exprs, maps);
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdisabled = disabled && ret;
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂexprs++;
> > @@ -343,6 +353,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;
> >
> > @@ -358,7 +370,7 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (expr)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_disable_sig(funcs, pdata->map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_disable_sig(funcs, pdata->maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂprios++;
> > @@ -377,7 +389,7 @@ int aspeed_pinmux_set_mux(struct pinctrl_dev *pctldev, unsigned int function,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENXIO;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_sig_expr_enable(expr, pdata->map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_sig_expr_enable(expr, pdata->maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> > ÂÂÂÂÂÂÂÂ}
> >
> > @@ -432,7 +444,7 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (aspeed_gpio_in_exprs(funcs))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> >
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_disable_sig(funcs, pdata->map))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂif (!aspeed_disable_sig(funcs, pdata->maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂprios++;
> > @@ -462,7 +474,7 @@ int aspeed_gpio_request_enable(struct pinctrl_dev *pctldev,
> > ÂÂÂÂÂÂÂÂÂ* If GPIO is not the lowest priority signal type, assume there is only
> > ÂÂÂÂÂÂÂÂÂ* one expression defined to enable the GPIO function
> > ÂÂÂÂÂÂÂÂÂ*/
> > -ÂÂÂÂÂÂÂif (!aspeed_sig_expr_enable(expr, pdata->map))
> > +ÂÂÂÂÂÂÂif (!aspeed_sig_expr_enable(expr, pdata->maps))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> >
> > ÂÂÂÂÂÂÂÂreturn 0;
> > @@ -481,10 +493,10 @@ int aspeed_pinctrl_probe(struct platform_device *pdev,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -ENODEV;
> > ÂÂÂÂÂÂÂÂ}
> >
> > -ÂÂÂÂÂÂÂpdata->map = syscon_node_to_regmap(parent->of_node);
> > -ÂÂÂÂÂÂÂif (IS_ERR(pdata->map)) {
> > +ÂÂÂÂÂÂÂpdata->maps[ASPEED_IP_SCU] = syscon_node_to_regmap(parent->of_node);
> > +ÂÂÂÂÂÂÂif (IS_ERR(pdata->maps[ASPEED_IP_SCU])) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_err(&pdev->dev, "No regmap for syscon pincontroller parent\n");
> > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(pdata->map);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂreturn PTR_ERR(pdata->maps[ASPEED_IP_SCU]);
> > ÂÂÂÂÂÂÂÂ}
> >
> > ÂÂÂÂÂÂÂÂpctl = pinctrl_register(pdesc, &pdev->dev, pdata);
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > index 3e72ef8c54bf..727728b86c07 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > @@ -232,6 +232,11 @@
> > Â * group.
> > Â */
> >
> > +#define ASPEED_IP_SCUÂÂ0
> > +#define ASPEED_IP_GFXÂÂ1
> > +#define ASPEED_IP_LPCHCÂÂÂÂÂÂÂÂ2
> > +#define ASPEED_NR_PINMUX_IPSÂÂÂ3
> > +
> > Â/*
> > Â * The "Multi-function Pins Mapping and Control" table in the SoC datasheet
> > Â * references registers by the device/offset mnemonic. The register macros
> > @@ -261,7 +266,9 @@
> > ÂÂÂ* 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
> > +ÂÂ* @ip: The IP block identifier, used as an index into the regmap array in
> > +ÂÂ*ÂÂÂÂÂÂstruct aspeed_pinctrl_data
> > +ÂÂ* @reg: The register offset with respect to the base address of the IP block
> > ÂÂÂ* @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,6 +277,7 @@
> > ÂÂÂ*ÂÂÂÂÂÂÂÂÂÂÂLSBs, not at the position of the mask.
> > ÂÂÂ*/
> > Âstruct aspeed_sig_desc {
> > +ÂÂÂÂÂÂÂunsigned int ip;
> > ÂÂÂÂÂÂÂÂunsigned int reg;
> > ÂÂÂÂÂÂÂÂu32 mask;
> > ÂÂÂÂÂÂÂÂu32 enable;
> > @@ -313,24 +321,30 @@ struct aspeed_pin_desc {
> >
> > Â/* Macro hell */
> >
> > +#define SIG_DESC_IP_BIT(ip, reg, idx, val) \
> > +ÂÂÂÂÂÂÂ{ ip, reg, BIT_MASK(idx), val, (((val) + 1) & 1) }
> > +
> > Â/**
> > - * Short-hand macro for describing a configuration enabled by the state of one
> > - * bit. The disable value is derived.
> > + * Short-hand macro for describing an SCU descriptor enabled by the state of
> > + * one bit. The disable value is derived.
> > Â *
> > Â * @reg: The signal's associated register, offset from base
> > Â * @idx: The signal's bit index in the register
> > Â * @val: The value (0 or 1) that enables the function
> > Â */
> > Â#define SIG_DESC_BIT(reg, idx, val) \
> > -ÂÂÂÂÂÂÂ{ reg, BIT_MASK(idx), val, (((val) + 1) & 1) }
> > +ÂÂÂÂÂÂÂSIG_DESC_IP_BIT(ASPEED_IP_SCU, reg, idx, val)
> > +
> > +#define SIG_DESC_IP_SET(ip, reg, idx) SIG_DESC_IP_BIT(ip, reg, idx, 1)
> >
> > Â/**
> > - * A further short-hand macro describing a configuration enabled with a set bit.
> > + * A further short-hand macro expanding to an SCU descriptor enabled by a set
> > + * bit.
> > Â *
> > - * @reg: The configuration's associated register, offset from base
> > - * @idx: The configuration's bit index in the register
> > + * @reg: The register, offset from base
> > + * @idx: The bit index in the register
> > Â */
> > -#define SIG_DESC_SET(reg, idx) SIG_DESC_BIT(reg, idx, 1)
> > +#define SIG_DESC_SET(reg, idx) SIG_DESC_IP_BIT(ASPEED_IP_SCU, reg, idx, 1)
> >
> > Â#define SIG_DESC_LIST_SYM(sig, func) sig_descs_ ## sig ## _ ## func
> > Â#define SIG_DESC_LIST_DECL(sig, func, ...) \
> > @@ -500,7 +514,7 @@ struct aspeed_pin_desc {
> > ÂÂÂÂÂÂÂÂMS_PIN_DECL_(pin, SIG_EXPR_LIST_PTR(gpio))
> >
> > Âstruct aspeed_pinctrl_data {
> > -ÂÂÂÂÂÂÂstruct regmap *map;
> > +ÂÂÂÂÂÂÂstruct regmap *maps[ASPEED_NR_PINMUX_IPS];
> >
> > ÂÂÂÂÂÂÂÂconst struct pinctrl_pin_desc *pins;
> > ÂÂÂÂÂÂÂÂconst unsigned int npins;
> > --
> > 2.7.4
> >

Attachment: signature.asc
Description: This is a digitally signed message part

Next message: Alex Williamson: "Re: [RFC 0/8] KVM PCIe/MSI passthrough on ARM/ARM64 (Alt II)"
Previous message: Andy Lutomirski: "Re: vmalloced stacks and scatterwalk_map_and_copy()"
In reply to: Joel Stanley: "Re: [PATCH v2 4/6] pinctrl: aspeed: Read and write bits in LPCHC and GFX controllers"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]