* Re: [PATCH] gpio: pl061: Pass irqchip when adding gpiochip
From: Manivannan Sadhasivam @ 2019-06-27 10:32 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-gpio, Bartosz Golaszewski, Sudeep Holla
In-Reply-To: <20190625111824.28482-1-linus.walleij@linaro.org>
On Tue, Jun 25, 2019 at 01:18:24PM +0200, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip.
>
> For chained irqchips this is a pretty straight-forward
> conversion.
>
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Looks good to me!
Acked-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Thanks,
Mani
> ---
> drivers/gpio/gpio-pl061.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
> index dc42571e6fdc..84af248f5172 100644
> --- a/drivers/gpio/gpio-pl061.c
> +++ b/drivers/gpio/gpio-pl061.c
> @@ -286,6 +286,7 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> {
> struct device *dev = &adev->dev;
> struct pl061 *pl061;
> + struct gpio_irq_chip *girq;
> int ret, irq;
>
> pl061 = devm_kzalloc(dev, sizeof(*pl061), GFP_KERNEL);
> @@ -313,10 +314,6 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> pl061->gc.parent = dev;
> pl061->gc.owner = THIS_MODULE;
>
> - ret = gpiochip_add_data(&pl061->gc, pl061);
> - if (ret)
> - return ret;
> -
> /*
> * irq_chip support
> */
> @@ -335,19 +332,24 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
> }
> pl061->parent_irq = irq;
>
> - ret = gpiochip_irqchip_add(&pl061->gc, &pl061->irq_chip,
> - 0, handle_bad_irq,
> - IRQ_TYPE_NONE);
> - if (ret) {
> - dev_info(&adev->dev, "could not add irqchip\n");
> + girq = &pl061->gc.irq;
> + girq->chip = &pl061->irq_chip;
> + girq->parent_handler = pl061_irq_handler;
> + girq->num_parents = 1;
> + girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
> + GFP_KERNEL);
> + if (!girq->parents)
> + return -ENOMEM;
> + girq->parents[0] = irq;
> + girq->default_type = IRQ_TYPE_NONE;
> + girq->handler = handle_bad_irq;
> +
> + ret = devm_gpiochip_add_data(dev, &pl061->gc, pl061);
> + if (ret)
> return ret;
> - }
> - gpiochip_set_chained_irqchip(&pl061->gc, &pl061->irq_chip,
> - irq, pl061_irq_handler);
>
> amba_set_drvdata(adev, pl061);
> - dev_info(&adev->dev, "PL061 GPIO chip @%pa registered\n",
> - &adev->res.start);
> + dev_info(dev, "PL061 GPIO chip %s registered\n", dev_name(dev));
>
> return 0;
> }
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH] gpio: ath79: Pass irqchip when adding gpiochip
From: Alban @ 2019-06-27 10:19 UTC (permalink / raw)
To: Linus Walleij; +Cc: Alban Bedel, linux-gpio, Bartosz Golaszewski, linux-mips
In-Reply-To: <20190625120030.16432-1-linus.walleij@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 587 bytes --]
On Tue, 25 Jun 2019 14:00:30 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip.
>
> For chained irqchips this is a pretty straight-forward
> conversion.
>
> Take this opportunity to add a local dev pointer and
> use devm_gpiochip_add() so we can get rid of the remove()
> callback altogether.
>
> Cc: Alban Bedel <albeu@free.fr>
> Cc: linux-mips@linux-mips.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Alban Bedel <albeu@free.fr>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: pinctrl: aspeed: Split bindings document in two
From: Joel Stanley @ 2019-06-27 4:07 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <226afe63-cc86-4920-abc1-025bdda32063@www.fastmail.com>
On Thu, 27 Jun 2019 at 04:02, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Thu, 27 Jun 2019, at 13:02, Joel Stanley wrote:
> > On Wed, 26 Jun 2019 at 07:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > Have one for each of the AST2400 and AST2500. The only thing that was
> > > common was the fact that both support ASPEED BMC SoCs.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 +++++++++++++++++++
> > > ...-aspeed.txt => aspeed,ast2500-pinctrl.txt} | 63 ++-------------
> > > 2 files changed, 85 insertions(+), 58 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > > rename Documentation/devicetree/bindings/pinctrl/{pinctrl-aspeed.txt => aspeed,ast2500-pinctrl.txt} (66%)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > > new file mode 100644
> > > index 000000000000..67e0325ccf2e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > > @@ -0,0 +1,80 @@
> > > +=============================
> > > +Aspeed AST2400 Pin Controller
> > > +=============================
> > > +
> > > +Required properties for the AST2400:
> > > +- compatible : Should be one of the following:
> > > + "aspeed,ast2400-pinctrl"
> > > + "aspeed,g4-pinctrl"
> > > +
> > > +The pin controller node should be the child of a syscon node with the required
> > > +property:
> > > +
> > > +- compatible : Should be one of the following:
> > > + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> > > + "aspeed,g4-scu", "syscon", "simple-mfd"
> >
> > I think we can use this as an opportunity to drop the unused g4-scu
> > compatible from the bindings. Similarly for the g5.
>
> I Wonder if we should eradicate that pattern for all the aspeed compatibles?
Yes. We've settled on ast2x00,aspeed-<foo> for most of them. If you're
aware of others we should remove them from the bindings.
I think we've stopped any new users of the gx style from getting merged.
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: pinctrl: aspeed: Split bindings document in two
From: Andrew Jeffery @ 2019-06-27 4:02 UTC (permalink / raw)
To: Joel Stanley
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <CACPK8Xfdd1ReAHr9f6zRbZ-WJRquDJsTdUQeT_JuEBhOzS8tig@mail.gmail.com>
On Thu, 27 Jun 2019, at 13:02, Joel Stanley wrote:
> On Wed, 26 Jun 2019 at 07:15, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Have one for each of the AST2400 and AST2500. The only thing that was
> > common was the fact that both support ASPEED BMC SoCs.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 +++++++++++++++++++
> > ...-aspeed.txt => aspeed,ast2500-pinctrl.txt} | 63 ++-------------
> > 2 files changed, 85 insertions(+), 58 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > rename Documentation/devicetree/bindings/pinctrl/{pinctrl-aspeed.txt => aspeed,ast2500-pinctrl.txt} (66%)
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > new file mode 100644
> > index 000000000000..67e0325ccf2e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > @@ -0,0 +1,80 @@
> > +=============================
> > +Aspeed AST2400 Pin Controller
> > +=============================
> > +
> > +Required properties for the AST2400:
> > +- compatible : Should be one of the following:
> > + "aspeed,ast2400-pinctrl"
> > + "aspeed,g4-pinctrl"
> > +
> > +The pin controller node should be the child of a syscon node with the required
> > +property:
> > +
> > +- compatible : Should be one of the following:
> > + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> > + "aspeed,g4-scu", "syscon", "simple-mfd"
>
> I think we can use this as an opportunity to drop the unused g4-scu
> compatible from the bindings. Similarly for the g5.
I Wonder if we should eradicate that pattern for all the aspeed compatibles?
>
> Acked-by: Joel Stanley <joel@jms.id.au>
Cheers,
Andrew
^ permalink raw reply
* Re: [PATCH 5/8] pinctrl: aspeed: Correct comment that is no longer true
From: Andrew Jeffery @ 2019-06-27 3:57 UTC (permalink / raw)
To: Joel Stanley
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <CACPK8XcPxJAOBAnKMKxtiG4Fkz8BPf8KtW1Kc3A9tU_emQviVg@mail.gmail.com>
On Thu, 27 Jun 2019, at 13:00, Joel Stanley wrote:
> On Wed, 26 Jun 2019 at 07:16, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > We have handled the GFX register case for quite some time now.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > drivers/pinctrl/aspeed/pinctrl-aspeed.h | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > index 4b06ddbc6aec..c5918c4a087c 100644
> > --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> > @@ -240,8 +240,7 @@
> > * opposed to naming them e.g. PINMUX_CTRL_[0-9]). Further, signal expressions
> > * reference registers beyond those dedicated to pinmux, such as the system
> > * reset control and MAC clock configuration registers. The AST2500 goes a step
>
> AST2600 too?
No mention of the GFX block in the pinctrl table for the 2600, it appears the pinmux
state is entirely determined by SCU registers.
>
> Acked-by: Joel Stanley <joel@jms.id.au>
Cheers,
Andrew
>
> > - * further and references registers in the graphics IP block, but that isn't
> > - * handled yet.
> > + * further and references registers in the graphics IP block.
> > */
> > #define SCU2C 0x2C /* Misc. Control Register */
> > #define SCU3C 0x3C /* System Reset Control/Status Register */
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [PATCH 2/8] dt-bindings: pinctrl: aspeed: Convert AST2400 bindings to json-schema
From: Andrew Jeffery @ 2019-06-27 3:55 UTC (permalink / raw)
To: Rob Herring
Cc: open list:GPIO SUBSYSTEM, Ryan Chen, Linus Walleij, Mark Rutland,
Joel Stanley, linux-aspeed, OpenBMC Maillist, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_JsqKXPzFYTHos-uvCUtBj-bcsNfrzt5GjxQ=PmgeXpp5J-A@mail.gmail.com>
On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote:
> On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > + The pin controller node should be the child of a syscon node with the
> > + required property:
> > +
> > + - compatible: Should be one of the following:
> > + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> > + "aspeed,g4-scu", "syscon", "simple-mfd"
> > +
> > + Refer to the the bindings described in
> > + Documentation/devicetree/bindings/mfd/syscon.txt
> > +
> > + For the AST2400 pinmux, each mux function has only one associated pin group.
> > + Each group is named by its function. The following values for the function
> > + and groups properties are supported:
> > +
> > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6
> > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2
> > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4
> > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1
> > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4
> > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK
> > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8
> > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1
> > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI
> > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU
> > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6
> > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24
> > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2
>
> This should be a schema. You need to define child nodes and list these
> as values for 'function' and 'group'. Ideally, the child nodes would
> have some sort of pattern, but if not, you can just match on '^.*$'
> under patternProperties.
The children don't have any pattern in their node name, which drives
me towards the '^.*$' pattern match, however, what I've found is that
I get the following errors for some of the relevant dts files:
```
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-names: ['default'] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: pinctrl-0: [[7, 8, 9, 10, 11, 12]] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: phandle: [[13]] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dt.yaml: $nodename: ['pinctrl'] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: compatible: ['aspeed,g4-pinctrl'] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-names: ['default'] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: pinctrl-0: [[9, 10, 11, 12]] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: phandle: [[13]] is not of type 'object'
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: $nodename: ['pinctrl'] is not of type 'object'
```
We shouldn't be expecting these properties in the child nodes, so
something is busted. Looking at processed-schema.yaml, we have:
```
- $filename: /home/andrew/src/linux/aspeed/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
$id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
patternProperties:
^.*$:
patternProperties:
^function|groups$:
allOf:
- {$ref: /schemas/types.yaml#/definitions/string}
- additionalItems: false
items:
enum: [ACPI, ADC0, ADC1, ADC10, ADC11, ADC12, ADC13, ADC14, ADC15, ADC2,
ADC3, ADC4, ADC5, ADC6, ADC7, ADC8, ADC9, BMCINT, DDCCLK, DDCDAT,
EXTRST, FLACK, FLBUSY, FLWP, GPID, GPID0, GPID2, GPID4, GPID6, GPIE0,
GPIE2, GPIE4, GPIE6, I2C10, I2C11, I2C12, I2C13, I2C14, I2C3, I2C4,
I2C5, I2C6, I2C7, I2C8, I2C9, LPCPD, LPCPME, LPCRST, LPCSMI, MAC1LINK,
MAC2LINK, MDIO1, MDIO2, NCTS1, NCTS2, NCTS3, NCTS4, NDCD1, NDCD2,
NDCD3, NDCD4, NDSR1, NDSR2, NDSR3, NDSR4, NDTR1, NDTR2, NDTR3, NDTR4,
NDTS4, NRI1, NRI2, NRI3, NRI4, NRTS1, NRTS2, NRTS3, OSCCLK, PWM0,
PWM1, PWM2, PWM3, PWM4, PWM5, PWM6, PWM7, RGMII1, RGMII2, RMII1, RMII2,
ROM16, ROM8, ROMCS1, ROMCS2, ROMCS3, ROMCS4, RXD1, RXD2, RXD3, RXD4,
SALT1, SALT2, SALT3, SALT4, SD1, SD2, SGPMCK, SGPMI, SGPMLD, SGPMO,
SGPSCK, SGPSI0, SGPSI1, SGPSLD, SIOONCTRL, SIOPBI, SIOPBO, SIOPWREQ,
SIOPWRGD, SIOS3, SIOS5, SIOSCI, SPI1, SPI1DEBUG, SPI1PASSTHRU, SPICS1,
TIMER3, TIMER4, TIMER5, TIMER6, TIMER7, TIMER8, TXD1, TXD2, TXD3,
TXD4, UART6, USB11D1, USB11H2, USB2D1, USB2H1, USBCKI, VGABIOS_ROM,
VGAHS, VGAVS, VPI18, VPI24, VPI30, VPO12, VPO24, WDTRST1, WDTRST2]
maxItems: 1
minItems: 1
type: array
pinctrl-[0-9]+: true
properties: {phandle: true, pinctrl-names: true, status: true}
type: object
pinctrl-[0-9]+: true
properties:
$nodename: true
compatible:
additionalItems: false
items:
- enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl']
maxItems: 1
minItems: 1
type: array
phandle: true
pinctrl-names: true
status: true
required: [compatible]
select:
properties:
compatible:
contains:
enum: ['aspeed,ast2400-pinctrl', 'aspeed,g4-pinctrl']
required: [compatible]
title: ASPEED AST2400 Pin Controller
```
`properties: {phandle: true, pinctrl-names: true, status: true}` has been
merged into my '^.*$' patternProperty, presumably partly from
pinctrl-consumer.yaml, and this seems to be the source of the bad
output. If as a hack I change my pattern to '^.*_default$' the problem
goes away as we no longer try to enforce the constraints on properties
provided by other bindings, but the problem is the node names are
largely freeform[1] (unless I enforce a naming constraint as part of my
bindings?).
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt?h=v5.2-rc6#n112
>
> BTW, You can put the names under a 'definitions' key and then use
> '$ref' to reference them from function and group to avoid duplicating
> the names. Or use patternProperties with '^(function|group)$'.
I've used the patternProperties approach above as I couldn't get the
definitions/$ref approach to work. I did the following:
```
definitions:
pinctrl-value:
allOf:
- $ref: "/schemas/types.yaml#/definitions/string"
- enum: [ "ACPI", "ADC0", "ADC1", "ADC10", "ADC11", "ADC12", "ADC13",
"ADC14", "ADC15", "ADC2", "ADC3", "ADC4", "ADC5", "ADC6", "ADC7",
"ADC8", "ADC9", "BMCINT", "DDCCLK", "DDCDAT", "EXTRST", "FLACK",
"FLBUSY", "FLWP", "GPID", "GPID0", "GPID2", "GPID4", "GPID6", "GPIE0",
"GPIE2", "GPIE4", "GPIE6", "I2C10", "I2C11", "I2C12", "I2C13", "I2C14",
"I2C3", "I2C4", "I2C5", "I2C6", "I2C7", "I2C8", "I2C9", "LPCPD",
"LPCPME", "LPCRST", "LPCSMI", "MAC1LINK", "MAC2LINK", "MDIO1", "MDIO2",
"NCTS1", "NCTS2", "NCTS3", "NCTS4", "NDCD1", "NDCD2", "NDCD3", "NDCD4",
"NDSR1", "NDSR2", "NDSR3", "NDSR4", "NDTR1", "NDTR2", "NDTR3", "NDTR4",
"NDTS4", "NRI1", "NRI2", "NRI3", "NRI4", "NRTS1", "NRTS2", "NRTS3",
"OSCCLK", "PWM0", "PWM1", "PWM2", "PWM3", "PWM4", "PWM5", "PWM6",
"PWM7", "RGMII1", "RGMII2", "RMII1", "RMII2", "ROM16", "ROM8",
"ROMCS1", "ROMCS2", "ROMCS3", "ROMCS4", "RXD1", "RXD2", "RXD3", "RXD4",
"SALT1", "SALT2", "SALT3", "SALT4", "SD1", "SD2", "SGPMCK", "SGPMI",
"SGPMLD", "SGPMO", "SGPSCK", "SGPSI0", "SGPSI1", "SGPSLD", "SIOONCTRL",
"SIOPBI", "SIOPBO", "SIOPWREQ", "SIOPWRGD", "SIOS3", "SIOS5", "SIOSCI",
"SPI1", "SPI1DEBUG", "SPI1PASSTHRU", "SPICS1", "TIMER3", "TIMER4",
"TIMER5", "TIMER6", "TIMER7", "TIMER8", "TXD1", "TXD2", "TXD3", "TXD4",
"UART6", "USB11D1", "USB11H2", "USB2D1", "USB2H1", "USBCKI",
"VGABIOS_ROM", "VGAHS", "VGAVS", "VPI18", "VPI24", "VPI30", "VPO12",
"VPO24", "WDTRST1", "WDTRST2" ]
patternProperties:
'^.*_default$':
type: object
properties:
function:
$ref: "#/definitions/pinctrl-value"
groups:
$ref: "#/definitions/pinctrl-value"
```
But it gave me output like:
```
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: wdtrst2_default:function: ['WDTRST2'] is not one of ['ACPI', 'ADC0', 'ADC1', 'ADC10', 'ADC11', 'ADC12', 'ADC13', 'ADC14', 'ADC15', 'ADC2', 'ADC3', 'ADC4', 'ADC5', 'ADC6', 'ADC7', 'ADC8', 'ADC9', 'BMCINT', 'DDCCLK', 'DDCDAT', 'EXTRST', 'FLACK', 'FLBUSY', 'FLWP', 'GPID', 'GPID0', 'GPID2', 'GPID4', 'GPID6', 'GPIE0', 'GPIE2', 'GPIE4', 'GPIE6', 'I2C10', 'I2C11', 'I2C12', 'I2C13', 'I2C14', 'I2C3', 'I2C4', 'I2C5', 'I2C6', 'I2C7', 'I2C8', 'I2C9', 'LPCPD', 'LPCPME', 'LPCRST', 'LPCSMI', 'MAC1LINK', 'MAC2LINK', 'MDIO1', 'MDIO2', 'NCTS1', 'NCTS2', 'NCTS3', 'NCTS4', 'NDCD1', 'NDCD2', 'NDCD3', 'NDCD4', 'NDSR1', 'NDSR2', 'NDSR3', 'NDSR4', 'NDTR1', 'NDTR2', 'NDTR3', 'NDTR4', 'NDTS4', 'NRI1', 'NRI2', 'NRI3', 'NRI4', 'NRTS1', 'NRTS2', 'NRTS3', 'OSCCLK', 'PWM0', 'PWM1', 'PWM2', 'PWM3', 'PWM4', 'PWM5', 'PWM6', 'PWM7', 'RGMII1', 'RGMII2', 'RMII1', 'RMII2', 'ROM16', 'ROM8', 'ROMCS1', 'ROMCS2', 'ROMCS3', 'ROMCS4', 'RXD1', 'RXD2', 'RXD3', 'RXD4', 'SALT1', 'SALT2', 'SALT3', 'SALT4', 'SD1', 'SD2', 'SGPMCK', 'SGPMI', 'SGPMLD', 'SGPMO', 'SGPSCK', 'SGPSI0', 'SGPSI1', 'SGPSLD', 'SIOONCTRL', 'SIOPBI', 'SIOPBO', 'SIOPWREQ', 'SIOPWRGD', 'SIOS3', 'SIOS5', 'SIOSCI', 'SPI1', 'SPI1DEBUG', 'SPI1PASSTHRU', 'SPICS1', 'TIMER3', 'TIMER4', 'TIMER5', 'TIMER6', 'TIMER7', 'TIMER8', 'TXD1', 'TXD2', 'TXD3', 'TXD4', 'UART6', 'USB11D1', 'USB11H2', 'USB2D1', 'USB2H1', 'USBCKI', 'VGABIOS_ROM', 'VGAHS', 'VGAVS', 'VPI18', 'VPI24', 'VPI30', 'VPO12', 'VPO24', 'WDTRST1', 'WDTRST2']
/home/andrew/src/linux/aspeed/arch/arm/boot/dts/aspeed-bmc-quanta-q71l.dt.yaml: wdtrst2_default:groups: ['WDTRST2'] is not one of ['ACPI', 'ADC0', 'ADC1', 'ADC10', 'ADC11', 'ADC12', 'ADC13', 'ADC14', 'ADC15', 'ADC2', 'ADC3', 'ADC4', 'ADC5', 'ADC6', 'ADC7', 'ADC8', 'ADC9', 'BMCINT', 'DDCCLK', 'DDCDAT', 'EXTRST', 'FLACK', 'FLBUSY', 'FLWP', 'GPID', 'GPID0', 'GPID2', 'GPID4', 'GPID6', 'GPIE0', 'GPIE2', 'GPIE4', 'GPIE6', 'I2C10', 'I2C11', 'I2C12', 'I2C13', 'I2C14', 'I2C3', 'I2C4', 'I2C5', 'I2C6', 'I2C7', 'I2C8', 'I2C9', 'LPCPD', 'LPCPME', 'LPCRST', 'LPCSMI', 'MAC1LINK', 'MAC2LINK', 'MDIO1', 'MDIO2', 'NCTS1', 'NCTS2', 'NCTS3', 'NCTS4', 'NDCD1', 'NDCD2', 'NDCD3', 'NDCD4', 'NDSR1', 'NDSR2', 'NDSR3', 'NDSR4', 'NDTR1', 'NDTR2', 'NDTR3', 'NDTR4', 'NDTS4', 'NRI1', 'NRI2', 'NRI3', 'NRI4', 'NRTS1', 'NRTS2', 'NRTS3', 'OSCCLK', 'PWM0', 'PWM1', 'PWM2', 'PWM3', 'PWM4', 'PWM5', 'PWM6', 'PWM7', 'RGMII1', 'RGMII2', 'RMII1', 'RMII2', 'ROM16', 'ROM8', 'ROMCS1', 'ROMCS2', 'ROMCS3', 'ROMCS4', 'RXD1', 'RXD2', 'RXD3', 'RXD4', 'SALT1', 'SALT2', 'SALT3', 'SALT4', 'SD1', 'SD2', 'SGPMCK', 'SGPMI', 'SGPMLD', 'SGPMO', 'SGPSCK', 'SGPSI0', 'SGPSI1', 'SGPSLD', 'SIOONCTRL', 'SIOPBI', 'SIOPBO', 'SIOPWREQ', 'SIOPWRGD', 'SIOS3', 'SIOS5', 'SIOSCI', 'SPI1', 'SPI1DEBUG', 'SPI1PASSTHRU', 'SPICS1', 'TIMER3', 'TIMER4', 'TIMER5', 'TIMER6', 'TIMER7', 'TIMER8', 'TXD1', 'TXD2', 'TXD3', 'TXD4', 'UART6', 'USB11D1', 'USB11H2', 'USB2D1', 'USB2H1', 'USBCKI', 'VGABIOS_ROM', 'VGAHS', 'VGAVS', 'VPI18', 'VPI24', 'VPI30', 'VPO12', 'VPO24', 'WDTRST1', 'WDTRST2']
```
Clearly I haven't got it quite right, but I'm not sure what's wrong with my approach. Can you tell me? It looks like the property is interpreted as a string-array rather than a string, but I'm not sure why.
Cheers,
Andrew
^ permalink raw reply
* Re: [PATCH 6/8] pinctrl: aspeed: Clarify comment about strapping W1C
From: Joel Stanley @ 2019-06-27 3:33 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <20190626071430.28556-7-andrew@aj.id.au>
On Wed, 26 Jun 2019 at 07:16, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Writes of 1 to SCU7C clear set bits in SCU70, the hardware strapping
> register. The information was correct if you squinted while reading, but
> hopefully switching the order of the registers as listed conveys it
> better.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Acked-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/pinctrl/aspeed/pinctrl-aspeed.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.c b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> index 4c775b8ffdc4..b510bb475851 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.c
> @@ -209,7 +209,7 @@ static int aspeed_sig_expr_set(const struct aspeed_sig_expr *expr,
> if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP2)
> continue;
>
> - /* On AST2500, Set bits in SCU7C are cleared from SCU70 */
> + /* On AST2500, Set bits in SCU70 are cleared from SCU7C */
> if (desc->ip == ASPEED_IP_SCU && desc->reg == HW_STRAP1) {
> unsigned int rev_id;
>
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 1/8] dt-bindings: pinctrl: aspeed: Split bindings document in two
From: Joel Stanley @ 2019-06-27 3:32 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <20190626071430.28556-2-andrew@aj.id.au>
On Wed, 26 Jun 2019 at 07:15, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Have one for each of the AST2400 and AST2500. The only thing that was
> common was the fact that both support ASPEED BMC SoCs.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 +++++++++++++++++++
> ...-aspeed.txt => aspeed,ast2500-pinctrl.txt} | 63 ++-------------
> 2 files changed, 85 insertions(+), 58 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> rename Documentation/devicetree/bindings/pinctrl/{pinctrl-aspeed.txt => aspeed,ast2500-pinctrl.txt} (66%)
>
> diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> new file mode 100644
> index 000000000000..67e0325ccf2e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> @@ -0,0 +1,80 @@
> +=============================
> +Aspeed AST2400 Pin Controller
> +=============================
> +
> +Required properties for the AST2400:
> +- compatible : Should be one of the following:
> + "aspeed,ast2400-pinctrl"
> + "aspeed,g4-pinctrl"
> +
> +The pin controller node should be the child of a syscon node with the required
> +property:
> +
> +- compatible : Should be one of the following:
> + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> + "aspeed,g4-scu", "syscon", "simple-mfd"
I think we can use this as an opportunity to drop the unused g4-scu
compatible from the bindings. Similarly for the g5.
Acked-by: Joel Stanley <joel@jms.id.au>
> +
> +Refer to the the bindings described in
> +Documentation/devicetree/bindings/mfd/syscon.txt
> +
> +Subnode Format
> +==============
> +
> +The required properties of pinmux child nodes are:
> +- function: the mux function to select
> +- groups : the list of groups to select with this function
> +
> +Required properties of pinconf child nodes are:
> +- groups: A list of groups to select (either this or "pins" must be
> + specified)
> +- pins : A list of ball names as strings, eg "D14" (either this or "groups"
> + must be specified)
> +
> +Optional properties of pinconf child nodes are:
> +- bias-disable : disable any pin bias
> +- bias-pull-down: pull down the pin
> +- drive-strength: sink or source at most X mA
> +
> +Definitions are as specified in
> +Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt, with any
> +further limitations as described above.
> +
> +For pinmux, each mux function has only one associated pin group. Each group is
> +named by its function. The following values for the function and groups
> +properties are supported:
> +
> +ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6
> +ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2
> +GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4
> +I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1
> +MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4
> +NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK PWM0
> +PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 ROMCS1
> +ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 SD2 SGPMCK
> +SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI SIOPBO SIOPWREQ
> +SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU SPICS1 TIMER3 TIMER4
> +TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 USB11D1 USB11H2 USB2D1
> +USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 VPI30 VPO12 VPO24 WDTRST1
> +WDTRST2
> +
> +Example
> +=======
> +
> +syscon: scu@1e6e2000 {
> + compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd";
> + reg = <0x1e6e2000 0x1a8>;
> +
> + pinctrl: pinctrl {
> + compatible = "aspeed,g4-pinctrl";
> +
> + pinctrl_i2c3_default: i2c3_default {
> + function = "I2C3";
> + groups = "I2C3";
> + };
> +
> + pinctrl_gpioh0_unbiased_default: gpioh0 {
> + pins = "A8";
> + bias-disable;
> + };
> + };
> +};
> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.txt
> similarity index 66%
> rename from Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt
> rename to Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.txt
> index 3b7266c7c438..2f16e401338a 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-aspeed.txt
> +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2500-pinctrl.txt
> @@ -1,14 +1,6 @@
> -======================
> -Aspeed Pin Controllers
> -======================
> -
> -The Aspeed SoCs vary in functionality inside a generation but have a common mux
> -device register layout.
> -
> -Required properties for g4:
> -- compatible : Should be one of the following:
> - "aspeed,ast2400-pinctrl"
> - "aspeed,g4-pinctrl"
> +=============================
> +Aspeed AST2500 Pin Controller
> +=============================
>
> Required properties for g5:
> - compatible : Should be one of the following:
> @@ -23,8 +15,6 @@ The pin controller node should be the child of a syscon node with the required
> property:
>
> - compatible : Should be one of the following:
> - "aspeed,ast2400-scu", "syscon", "simple-mfd"
> - "aspeed,g4-scu", "syscon", "simple-mfd"
> "aspeed,ast2500-scu", "syscon", "simple-mfd"
> "aspeed,g5-scu", "syscon", "simple-mfd"
>
> @@ -57,24 +47,6 @@ For pinmux, each mux function has only one associated pin group. Each group is
> named by its function. The following values for the function and groups
> properties are supported:
>
> -aspeed,ast2400-pinctrl, aspeed,g4-pinctrl:
> -
> -ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6
> -ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2
> -GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4
> -I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1
> -MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4
> -NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK PWM0
> -PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8 ROMCS1
> -ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1 SD2 SGPMCK
> -SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI SIOPBO SIOPWREQ
> -SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU SPICS1 TIMER3 TIMER4
> -TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6 USB11D1 USB11H2 USB2D1
> -USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24 VPI30 VPO12 VPO24 WDTRST1
> -WDTRST2
> -
> -aspeed,ast2500-pinctrl, aspeed,g5-pinctrl:
> -
> ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6
> ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT ESPI FWSPICS1 FWSPICS2 GPID0 GPID2 GPID4
> GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4 I2C5 I2C6
> @@ -90,33 +62,8 @@ SPI2CS1 SPI2MISO SPI2MOSI TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2
> TXD3 TXD4 UART6 USB11BHID USB2AD USB2AH USB2BD USB2BH USBCKI VGABIOSROM VGAHS
> VGAVS VPI24 VPO WDTRST1 WDTRST2
>
> -Examples
> -========
> -
> -g4 Example
> -----------
> -
> -syscon: scu@1e6e2000 {
> - compatible = "aspeed,ast2400-scu", "syscon", "simple-mfd";
> - reg = <0x1e6e2000 0x1a8>;
> -
> - pinctrl: pinctrl {
> - compatible = "aspeed,g4-pinctrl";
> -
> - pinctrl_i2c3_default: i2c3_default {
> - function = "I2C3";
> - groups = "I2C3";
> - };
> -
> - pinctrl_gpioh0_unbiased_default: gpioh0 {
> - pins = "A8";
> - bias-disable;
> - };
> - };
> -};
> -
> -g5 Example
> -----------
> +Example
> +=======
>
> ahb {
> apb {
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 5/8] pinctrl: aspeed: Correct comment that is no longer true
From: Joel Stanley @ 2019-06-27 3:30 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-gpio, Ryan Chen, Linus Walleij, Rob Herring, Mark Rutland,
linux-aspeed, OpenBMC Maillist, devicetree, Linux ARM,
Linux Kernel Mailing List
In-Reply-To: <20190626071430.28556-6-andrew@aj.id.au>
On Wed, 26 Jun 2019 at 07:16, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> We have handled the GFX register case for quite some time now.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> drivers/pinctrl/aspeed/pinctrl-aspeed.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed.h b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> index 4b06ddbc6aec..c5918c4a087c 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed.h
> @@ -240,8 +240,7 @@
> * opposed to naming them e.g. PINMUX_CTRL_[0-9]). Further, signal expressions
> * reference registers beyond those dedicated to pinmux, such as the system
> * reset control and MAC clock configuration registers. The AST2500 goes a step
AST2600 too?
Acked-by: Joel Stanley <joel@jms.id.au>
> - * further and references registers in the graphics IP block, but that isn't
> - * handled yet.
> + * further and references registers in the graphics IP block.
> */
> #define SCU2C 0x2C /* Misc. Control Register */
> #define SCU3C 0x3C /* System Reset Control/Status Register */
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 0/8] pinctrl: aspeed: Preparation for AST2600
From: Andrew Jeffery @ 2019-06-27 1:08 UTC (permalink / raw)
To: Linus Walleij
Cc: open list:GPIO SUBSYSTEM, Ryan Chen, Rob Herring, Mark Rutland,
Joel Stanley, linux-aspeed, OpenBMC Maillist,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux ARM, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdboxjMmeb8feffyG5JJ7fGPR6hqC8sc+XV5We3TC__LXg@mail.gmail.com>
On Wed, 26 Jun 2019, at 17:25, Linus Walleij wrote:
> On Wed, Jun 26, 2019 at 9:15 AM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> > The ASPEED AST2600 is in the pipeline, and we have enough information to start
> > preparing to upstream support for it. This series lays some ground work;
> > splitting the bindings and dicing the implementation up a little further to
> > facilitate differences between the 2600 and previous SoC generations.
>
> All looks good to me, but Rob should have a glance at the DT bindings
> and YAML syntax before I proceed to apply them.
Thanks for the quick review. Rob's responded, looks like I'll need to send a v2 at
least. Might need a hand sorting out describing generic pinctrl dt bits (subnodes
with function and group properties).
Cheers,
Andrew
^ permalink raw reply
* Re: [PATCH 1/2] pinctrl: remove unneeded #ifdef around declarations
From: Masahiro Yamada @ 2019-06-27 0:59 UTC (permalink / raw)
To: Linus Walleij; +Cc: open list:GPIO SUBSYSTEM, linux-kernel@vger.kernel.org
In-Reply-To: <CACRpkdb1MySnzCVGb6v1KovmgJtagKeSe+mrPvsVOJz_s198eA@mail.gmail.com>
On Tue, Jun 25, 2019 at 5:52 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hi Masahiro,
>
> thanks for your patch. For some reason I managed to pick up
> patch 2 before patch 1. I applied this now with some fuzzing.
> (Please check the result.)
>
> On Thu, Jun 13, 2019 at 3:55 AM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>
> > What is the point in surrounding the whole of declarations with
> > ifdef like this?
>
> I don't know if it is generally good to have phrases posed as
> questions in a commit message, we prefer to have statements
> about the change not a polemic dialog.
>
> > #ifdef CONFIG_FOO
> > int foo(void);
> > #endif
> >
> > If CONFIG_FOO is not defined, all callers of foo() will fail
> > with implicit declaration errors since the top Makefile adds
> > -Werror-implicit-function-declaration to KBUILD_CFLAGS.
>
> Maybe this flag was not in the top Makefile when the #ifdefs
> where introduced?
>
> > This breaks the build earlier when you are doing something wrong.
> > That's it.
>
> Good idea.
>
> > Anyway, it will fail to link since the definition of foo() is not
> > compiled.
> >
> > In summary, these ifdef are unneeded.
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Pushing this to the zeroday builders and let's see what happens!
Sorry for my terrible commit message and offending response.
I appended a new commit message below.
If you are OK with rebasing, please consider replacement.
--------------------------->8----------------------------------
pinctrl: remove less important #ifdef around declarations
The whole declarations in these headers are surrounded by #ifdef.
As far as I understood, the motivation of this is probably to break
the build earlier if a driver misses to select or depend on correct
CONFIG options in Kconfig.
Since commit 94bed2a9c4ae ("Add -Werror-implicit-function-declaration")
no one cannot call functions that have not been declared.
So, I see some benefit in doing this in the cost of uglier headers.
In reality, it would not be so easy to catch missed 'select' or
'depends on' because PINCTRL, PINMUX, etc. are already selected by
someone else eventually. So, this kind of error, if any, will be
caught by randconfig bots.
In summary, I am not a big fan of deep #ifdef nesting, and this
does not matter for normal developers. The code readability wins.
--------------------------->8----------------------------------
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* Re: [PATCH 2/8] dt-bindings: pinctrl: aspeed: Convert AST2400 bindings to json-schema
From: Andrew Jeffery @ 2019-06-27 0:44 UTC (permalink / raw)
To: Rob Herring
Cc: open list:GPIO SUBSYSTEM, Ryan Chen, Linus Walleij, Mark Rutland,
Joel Stanley, linux-aspeed, OpenBMC Maillist, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_JsqKXPzFYTHos-uvCUtBj-bcsNfrzt5GjxQ=PmgeXpp5J-A@mail.gmail.com>
On Wed, 26 Jun 2019, at 23:17, Rob Herring wrote:
> On Wed, Jun 26, 2019 at 1:21 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Convert ASPEED pinctrl bindings to DT schema format using json-schema
>
> BTW, ASPEED is one of the remaining platforms needing the top-level
> board bindings converted.
Okay, I'll put together patches to fix that.
>
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > .../pinctrl/aspeed,ast2400-pinctrl.txt | 80 -------------------
> > .../pinctrl/aspeed,ast2400-pinctrl.yaml | 73 +++++++++++++++++
> > 2 files changed, 73 insertions(+), 80 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.txt
> > create mode 100644 Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
>
> > diff --git a/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..3b8cf3e51506
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/aspeed,ast2400-pinctrl.yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: GPL-2.0+
>
> Do you have rights to change the license?
Where are you coming from with this question? The bindings previously didn't list a
license, is there some implicit license for them? I would have thought it was GPL-2.0?
IBM's (my employer's) preferred contribution license is GPL 2.0-or-later, so I was just
adding the SPDX marker to clarify.
> If so, the preference is to
> dual license with (GPL-2.0 OR BSD-2-Clause).
You're asking if I have the power to relicense so I can dual license it this way?
>
> BTW, '-or-later' is the preferred form over '+'.
Thanks for the pointer.
>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/aspeed,ast2400-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED AST2400 Pin Controller
> > +
> > +maintainers:
> > + - Andrew Jeffery <andrew@aj.id.au>
> > +
> > +properties:
> > + compatible:
> > + oneOf:
> > + - items:
> > + - enum:
> > + - aspeed,ast2400-pinctrl
> > + - items:
> > + - enum:
> > + - aspeed,g4-pinctrl
>
> This can be simplified to:
>
> compatible:
> enum:
> - aspeed,ast2400-pinctrl
> - aspeed,g4-pinctrl
Ah, that makes more sense, I think I was thrown by some details of the example.
>
> > +
> > +required:
> > + - compatible
> > +
> > +description: |+
>
> description goes before properties.
Okay. I wouldn't have thought the ordering mattered. Is this just a preference?
The tools seemed to run fine as is.
I'll re-order it regardless.
>
> > + The pin controller node should be the child of a syscon node with the
> > + required property:
> > +
> > + - compatible: Should be one of the following:
> > + "aspeed,ast2400-scu", "syscon", "simple-mfd"
> > + "aspeed,g4-scu", "syscon", "simple-mfd"
> > +
> > + Refer to the the bindings described in
> > + Documentation/devicetree/bindings/mfd/syscon.txt
> > +
> > + For the AST2400 pinmux, each mux function has only one associated pin group.
> > + Each group is named by its function. The following values for the function
> > + and groups properties are supported:
> > +
> > + ACPI ADC0 ADC1 ADC10 ADC11 ADC12 ADC13 ADC14 ADC15 ADC2 ADC3 ADC4 ADC5 ADC6
> > + ADC7 ADC8 ADC9 BMCINT DDCCLK DDCDAT EXTRST FLACK FLBUSY FLWP GPID GPID0 GPID2
> > + GPID4 GPID6 GPIE0 GPIE2 GPIE4 GPIE6 I2C10 I2C11 I2C12 I2C13 I2C14 I2C3 I2C4
> > + I2C5 I2C6 I2C7 I2C8 I2C9 LPCPD LPCPME LPCRST LPCSMI MAC1LINK MAC2LINK MDIO1
> > + MDIO2 NCTS1 NCTS2 NCTS3 NCTS4 NDCD1 NDCD2 NDCD3 NDCD4 NDSR1 NDSR2 NDSR3 NDSR4
> > + NDTR1 NDTR2 NDTR3 NDTR4 NDTS4 NRI1 NRI2 NRI3 NRI4 NRTS1 NRTS2 NRTS3 OSCCLK
> > + PWM0 PWM1 PWM2 PWM3 PWM4 PWM5 PWM6 PWM7 RGMII1 RGMII2 RMII1 RMII2 ROM16 ROM8
> > + ROMCS1 ROMCS2 ROMCS3 ROMCS4 RXD1 RXD2 RXD3 RXD4 SALT1 SALT2 SALT3 SALT4 SD1
> > + SD2 SGPMCK SGPMI SGPMLD SGPMO SGPSCK SGPSI0 SGPSI1 SGPSLD SIOONCTRL SIOPBI
> > + SIOPBO SIOPWREQ SIOPWRGD SIOS3 SIOS5 SIOSCI SPI1 SPI1DEBUG SPI1PASSTHRU
> > + SPICS1 TIMER3 TIMER4 TIMER5 TIMER6 TIMER7 TIMER8 TXD1 TXD2 TXD3 TXD4 UART6
> > + USB11D1 USB11H2 USB2D1 USB2H1 USBCKI VGABIOS_ROM VGAHS VGAVS VPI18 VPI24
> > + VPI30 VPO12 VPO24 WDTRST1 WDTRST2
>
> This should be a schema.
Yeah, I covered this in my cover letter. I was hoping to get away without
that for the moment as this seems like the first pinctrl binding to be
converted, however if you insist...
> You need to define child nodes and list these
> as values for 'function' and 'group'. Ideally, the child nodes would
> have some sort of pattern, but if not, you can just match on '^.*$'
> under patternProperties.
>
> BTW, You can put the names under a 'definitions' key and then use
> '$ref' to reference them from function and group to avoid duplicating
> the names. Or use patternProperties with '^(function|group)$'.
Okay, I'll take some time to digest this while looking at the documentation.
>
> Similar comments apply to AST2500 binding.
Yes, will fix that too.
Thanks for the prompt review!
Andrew
^ permalink raw reply
* Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains
From: Lina Iyer @ 2019-06-26 21:09 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-gpio, Bartosz Golaszewski, Thomas Gleixner, Marc Zyngier,
Jon Hunter, Sowjanya Komatineni, Bitan Biswas, linux-tegra,
David Daney, Masahiro Yamada, Brian Masney, Thierry Reding
In-Reply-To: <20190624132531.6184-1-linus.walleij@linaro.org>
Thanks for the patch Linus. I was running into the warning in
gpiochip_set_irq_hooks(), because it was called from two places.
Hopefully, this will fix that as well. I will give it a try.
On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:
>Hierarchical IRQ domains can be used to stack different IRQ
>controllers on top of each other.
>
>Bring hierarchical IRQ domains into the GPIOLIB core with the
>following basic idea:
>
>Drivers that need their interrupts handled hierarchically
>specify a callback to translate the child hardware IRQ and
>IRQ type for each GPIO offset to a parent hardware IRQ and
>parent hardware IRQ type.
>
>Users have to pass the callback, fwnode, and parent irqdomain
>before calling gpiochip_irqchip_add().
>
>We use the new method of just filling in the struct
>gpio_irq_chip before adding the gpiochip for all hierarchical
>irqchips of this type.
>
>The code path for device tree is pretty straight-forward,
>while the code path for old boardfiles or anything else will
>be more convoluted requireing upfront allocation of the
>interrupts when adding the chip.
>
>One specific use-case where this can be useful is if a power
>management controller has top-level controls for wakeup
>interrupts. In such cases, the power management controller can
>be a parent to other interrupt controllers and program
>additional registers when an IRQ has its wake capability
>enabled or disabled.
>
>The hierarchical irqchip helper code will only be available
>when IRQ_DOMAIN_HIERARCHY is selected to GPIO chips using
>this should select or depend on that symbol. When using
>hierarchical IRQs, the parent interrupt controller must
>also be hierarchical all the way up to the top interrupt
>controller wireing directly into the CPU, so on systems
>that do not have this we can get rid of all the extra
>code for supporting hierarchical irqs.
>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Marc Zyngier <marc.zyngier@arm.com>
>Cc: Lina Iyer <ilina@codeaurora.org>
>Cc: Jon Hunter <jonathanh@nvidia.com>
>Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
>Cc: Bitan Biswas <bbiswas@nvidia.com>
>Cc: linux-tegra@vger.kernel.org
>Cc: David Daney <david.daney@cavium.com>
>Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>Cc: Brian Masney <masneyb@onstation.org>
>Signed-off-by: Thierry Reding <treding@nvidia.com>
>Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>---
>ChangeLog RFC->v1:
>- Tested on real hardware
>- Incorporate Thierry's idea to have a translation callback.
> He was right about this approach, I was wrong in insisting
> on IRQ maps.
>---
> Documentation/driver-api/gpio/driver.rst | 120 ++++++++--
> drivers/gpio/gpiolib.c | 285 ++++++++++++++++++++++-
> include/linux/gpio/driver.h | 46 ++++
> 3 files changed, 426 insertions(+), 25 deletions(-)
>
>diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
>index 1ce7fcd0f989..3099c7fbefdb 100644
>--- a/Documentation/driver-api/gpio/driver.rst
>+++ b/Documentation/driver-api/gpio/driver.rst
>@@ -259,7 +259,7 @@ most often cascaded off a parent interrupt controller, and in some special
> cases the GPIO logic is melded with a SoC's primary interrupt controller.
>
> The IRQ portions of the GPIO block are implemented using an irq_chip, using
>-the header <linux/irq.h>. So basically such a driver is utilizing two sub-
>+the header <linux/irq.h>. So this combined driver is utilizing two sub-
> systems simultaneously: gpio and irq.
>
> It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
>@@ -391,14 +391,108 @@ Infrastructure helpers for GPIO irqchips
> ----------------------------------------
>
> To help out in handling the set-up and management of GPIO irqchips and the
>-associated irqdomain and resource allocation callbacks, the gpiolib has
>-some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
>-symbol:
>-
>-- gpiochip_irqchip_add(): adds a chained cascaded irqchip to a gpiochip. It
>- will pass the struct gpio_chip* for the chip to all IRQ callbacks, so the
>- callbacks need to embed the gpio_chip in its state container and obtain a
>- pointer to the container using container_of().
>+associated irqdomain and resource allocation callbacks. These are activated
>+by selecting the Kconfig symbol GPIOLIB_IRQCHIP. If the symbol
>+IRQ_DOMAIN_HIERARCHY is also selected, hierarchical helpers will also be
>+provided. A big portion of overhead code will be managed by gpiolib,
>+under the assumption that your interrupts are 1-to-1-mapped to the
>+GPIO line index:
>+
>+ GPIO line offset Hardware IRQ
>+ 0 0
>+ 1 1
>+ 2 2
>+ ... ...
>+ ngpio-1 ngpio-1
>+
>+If some GPIO lines do not have corresponding IRQs, the bitmask valid_mask
>+and the flag need_valid_mask in gpio_irq_chip can be used to mask off some
>+lines as invalid for associating with IRQs.
>+
>+The preferred way to set up the helpers is to fill in the
>+struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
>+If you do this, the additional irq_chip will be set up by gpiolib at the
>+same time as setting up the rest of the GPIO functionality. The following
>+is a typical example of a cascaded interrupt handler using gpio_irq_chip:
>+
>+ /* Typical state container with dynamic irqchip */
>+ struct my_gpio {
>+ struct gpio_chip gc;
>+ struct irq_chip irq;
>+ };
>+
>+ int irq; /* from platform etc */
>+ struct my_gpio *g;
>+ struct gpio_irq_chip *girq
>+
>+ /* Set up the irqchip dynamically */
>+ g->irq.name = "my_gpio_irq";
>+ g->irq.irq_ack = my_gpio_ack_irq;
>+ g->irq.irq_mask = my_gpio_mask_irq;
>+ g->irq.irq_unmask = my_gpio_unmask_irq;
>+ g->irq.irq_set_type = my_gpio_set_irq_type;
>+
>+ /* Get a pointer to the gpio_irq_chip */
>+ girq = &g->gc.irq;
>+ girq->chip = &g->irq;
>+ girq->parent_handler = ftgpio_gpio_irq_handler;
>+ girq->num_parents = 1;
>+ girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>+ GFP_KERNEL);
Could this be folded into the gpiolib?
>+ if (!girq->parents)
>+ return -ENOMEM;
>+ girq->default_type = IRQ_TYPE_NONE;
>+ girq->handler = handle_bad_irq;
>+ girq->parents[0] = irq;
>+
>+ return devm_gpiochip_add_data(dev, &g->gc, g);
>+
>+The helper support using hierarchical interrupt controllers as well.
>+In this case the typical set-up will look like this:
>+
>+ /* Typical state container with dynamic irqchip */
>+ struct my_gpio {
>+ struct gpio_chip gc;
>+ struct irq_chip irq;
>+ struct fwnode_handle *fwnode;
>+ };
>+
>+ int irq; /* from platform etc */
>+ struct my_gpio *g;
>+ struct gpio_irq_chip *girq
>+
>+ /* Set up the irqchip dynamically */
>+ g->irq.name = "my_gpio_irq";
>+ g->irq.irq_ack = my_gpio_ack_irq;
>+ g->irq.irq_mask = my_gpio_mask_irq;
>+ g->irq.irq_unmask = my_gpio_unmask_irq;
>+ g->irq.irq_set_type = my_gpio_set_irq_type;
>+
>+ /* Get a pointer to the gpio_irq_chip */
>+ girq = &g->gc.irq;
>+ girq->chip = &g->irq;
>+ girq->default_type = IRQ_TYPE_NONE;
>+ girq->handler = handle_bad_irq;
>+ girq->fwnode = g->fwnode;
>+ girq->parent_domain = parent;
>+ girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
>+
Should be the necessary, if the driver implements it's own .alloc?
>+ return devm_gpiochip_add_data(dev, &g->gc, g);
>+
>+As you can see pretty similar, but you do not supply a parent handler for
>+the IRQ, instead a parent irqdomain, an fwnode for the hardware and
>+a funcion .child_to_parent_hwirq() that has the purpose of looking up
>+the parent hardware irq from a child (i.e. this gpio chip) hardware irq.
>+As always it is good to look at examples in the kernel tree for advice
>+on how to find the required pieces.
>+
>+The old way of adding irqchips to gpiochips after registration is also still
>+available but we try to move away from this:
>+
>+- DEPRECATED: gpiochip_irqchip_add(): adds a chained cascaded irqchip to a
>+ gpiochip. It will pass the struct gpio_chip* for the chip to all IRQ
>+ callbacks, so the callbacks need to embed the gpio_chip in its state
>+ container and obtain a pointer to the container using container_of().
> (See Documentation/driver-model/design-patterns.txt)
>
> - gpiochip_irqchip_add_nested(): adds a nested cascaded irqchip to a gpiochip,
>@@ -406,10 +500,10 @@ symbol:
> cascaded irq has to be handled by a threaded interrupt handler.
> Apart from that it works exactly like the chained irqchip.
>
>-- gpiochip_set_chained_irqchip(): sets up a chained cascaded irq handler for a
>- gpio_chip from a parent IRQ and passes the struct gpio_chip* as handler
>- data. Notice that we pass is as the handler data, since the irqchip data is
>- likely used by the parent irqchip.
>+- DEPRECATED: gpiochip_set_chained_irqchip(): sets up a chained cascaded irq
>+ handler for a gpio_chip from a parent IRQ and passes the struct gpio_chip*
>+ as handler data. Notice that we pass is as the handler data, since the
>+ irqchip data is likely used by the parent irqchip.
>
> - gpiochip_set_nested_irqchip(): sets up a nested cascaded irq handler for a
> gpio_chip from a parent IRQ. As the parent IRQ has usually been
>diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>index e013d417a936..af72ffa02963 100644
>--- a/drivers/gpio/gpiolib.c
>+++ b/drivers/gpio/gpiolib.c
>@@ -1718,6 +1718,240 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip,
> }
> EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip);
>
>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>+
>+/**
>+ * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip
>+ * to a gpiochip
>+ * @gc: the gpiochip to set the irqchip hierarchical handler to
>+ * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt
>+ * will then percolate up to the parent
>+ */
>+static void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gc,
>+ struct irq_chip *irqchip)
>+{
>+ /* DT will deal with mapping each IRQ as we go along */
>+ if (is_of_node(gc->irq.fwnode))
>+ return;
>+
>+ /*
>+ * This is for legacy and boardfile "irqchip" fwnodes: allocate
>+ * irqs upfront instead of dynamically since we don't have the
>+ * dynamic type of allocation that hardware description languages
>+ * provide. Once all GPIO drivers using board files are gone from
>+ * the kernel we can delete this code, but for a transitional period
>+ * it is necessary to keep this around.
>+ */
>+ if (is_fwnode_irqchip(gc->irq.fwnode)) {
>+ int i;
>+ int ret;
>+
>+ for (i = 0; i < gc->ngpio; i++) {
>+ struct irq_fwspec fwspec;
>+ unsigned int parent_hwirq;
>+ unsigned int parent_type;
>+ struct gpio_irq_chip *girq = &gc->irq;
>+
>+ /*
>+ * We call the child to parent translation function
>+ * only to check if the child IRQ is valid or not.
>+ * Just pick the rising edge type here as that is what
>+ * we likely need to support.
>+ */
>+ ret = girq->child_to_parent_hwirq(gc, i,
>+ IRQ_TYPE_EDGE_RISING,
>+ &parent_hwirq,
>+ &parent_type);
>+ if (ret) {
>+ chip_err(gc, "skip set-up on hwirq %d\n",
>+ i);
>+ continue;
>+ }
>+
>+ fwspec.fwnode = gc->irq.fwnode;
>+ /* This is the hwirq for the GPIO line side of things */
>+ fwspec.param[0] = i;
>+ /* Just pick something */
>+ fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>+ fwspec.param_count = 2;
>+ ret = __irq_domain_alloc_irqs(gc->irq.domain,
>+ /* just pick something */
>+ -1,
>+ 1,
>+ NUMA_NO_NODE,
>+ &fwspec,
>+ false,
>+ NULL);
>+ if (ret < 0) {
>+ chip_err(gc,
>+ "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
>+ i, parent_hwirq,
>+ ret);
>+ }
>+ }
>+ }
>+
>+ chip_err(gc, "%s unknown fwnode type proceed anyway\n", __func__);
>+
>+ return;
>+}
>+
>+static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
>+ struct irq_fwspec *fwspec,
>+ unsigned long *hwirq,
>+ unsigned int *type)
>+{
>+ /* We support standard DT translation */
>+ if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
>+ return irq_domain_translate_twocell(d, fwspec, hwirq, type);
>+ }
>+
>+ /* This is for board files and others not using DT */
>+ if (is_fwnode_irqchip(fwspec->fwnode)) {
>+ int ret;
>+
>+ ret = irq_domain_translate_twocell(d, fwspec, hwirq, type);
>+ if (ret)
>+ return ret;
>+ WARN_ON(*type == IRQ_TYPE_NONE);
>+ return 0;
>+ }
>+ return -EINVAL;
>+}
>+
>+static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
>+ unsigned int irq,
>+ unsigned int nr_irqs,
>+ void *data)
>+{
>+ struct gpio_chip *gc = d->host_data;
>+ irq_hw_number_t hwirq;
>+ unsigned int type = IRQ_TYPE_NONE;
>+ struct irq_fwspec *fwspec = data;
>+ int ret;
>+ int i;
>+
>+ chip_info(gc, "called %s\n", __func__);
>+
>+ ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
>+ if (ret)
>+ return ret;
>+
>+ chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
>+ irq, irq + nr_irqs - 1,
>+ hwirq, hwirq + nr_irqs - 1);
>+
>+ for (i = 0; i < nr_irqs; i++) {
>+ struct irq_fwspec parent_fwspec;
>+ unsigned int parent_hwirq;
>+ unsigned int parent_type;
>+ struct gpio_irq_chip *girq = &gc->irq;
>+
>+ ret = girq->child_to_parent_hwirq(gc, hwirq, type,
>+ &parent_hwirq, &parent_type);
>+ if (ret) {
>+ chip_err(gc, "can't look up hwirq %lu\n", hwirq);
>+ return ret;
>+ }
>+ chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
>+
>+ /*
>+ * We set handle_bad_irq because the .set_type() should
>+ * always be invoked and set the right type of handler.
>+ */
>+ irq_domain_set_info(d,
>+ irq + i,
>+ hwirq + i,
>+ gc->irq.chip,
>+ gc,
>+ handle_bad_irq,
>+ NULL, NULL);
>+ irq_set_probe(irq + i);
>+
>+ /*
>+ * Create a IRQ fwspec to send up to the parent irqdomain:
>+ * specify the hwirq we address on the parent and tie it
>+ * all together up the chain.
>+ */
>+ parent_fwspec.fwnode = d->parent->fwnode;
>+ parent_fwspec.param_count = 2;
>+ parent_fwspec.param[0] = parent_hwirq;
>+ /* This parent only handles asserted level IRQs */
>+ parent_fwspec.param[1] = parent_type;
>+ chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
>+ irq + i, parent_hwirq);
>+ ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
>+ &parent_fwspec);
>+ if (ret)
>+ chip_err(gc,
>+ "failed to allocate parent hwirq %d for hwirq %lu\n",
>+ parent_hwirq, hwirq);
>+ }
>+
>+ return 0;
>+}
>+
>+static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
>+ .activate = gpiochip_irq_domain_activate,
>+ .deactivate = gpiochip_irq_domain_deactivate,
>+ .translate = gpiochip_hierarchy_irq_domain_translate,
>+ .alloc = gpiochip_hierarchy_irq_domain_alloc,
>+ .free = irq_domain_free_irqs_common,
>+};
>+
>+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>+{
>+ if (!gc->irq.parent_domain) {
>+ chip_err(gc, "missing parent irqdomain\n");
>+ return -EINVAL;
>+ }
>+
>+ if (!gc->irq.parent_domain ||
>+ !gc->irq.child_to_parent_hwirq ||
This should probably be validated if the .ops have not been set.
>+ !gc->irq.fwnode) {
>+ chip_err(gc, "missing irqdomain vital data\n");
>+ return -EINVAL;
>+ }
>+
>+ gc->irq.domain = irq_domain_create_hierarchy(
>+ gc->irq.parent_domain,
>+ IRQ_DOMAIN_FLAG_HIERARCHY,
>+ gc->ngpio,
>+ gc->irq.fwnode,
>+ &gpiochip_hierarchy_domain_ops,
>+ gc);
>+
>+ if (!gc->irq.domain) {
>+ chip_err(gc, "failed to add hierarchical domain\n");
>+ return -EINVAL;
>+ }
>+
>+ gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
>+
>+ chip_info(gc, "set up hierarchical irqdomain\n");
>+
>+ return 0;
>+}
>+
>+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
>+{
>+ return !!gc->irq.parent_domain;
>+}
>+
>+#else
>+
>+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>+{
>+ return -EINVAL;
>+}
>+
>+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
>+{
>+ return false;
>+}
>+
>+#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
>+
> /**
> * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
> * @d: the irqdomain used by this irqchip
>@@ -1786,6 +2020,11 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
> .xlate = irq_domain_xlate_twocell,
> };
>
>+/*
>+ * TODO: move these activate/deactivate in under the hierarchicial
>+ * irqchip implementation as static once SPMI and SSBI (all external
>+ * users) are phased over.
>+ */
> /**
> * gpiochip_irq_domain_activate() - Lock a GPIO to be used as an IRQ
> * @domain: The IRQ domain used by this IRQ chip
>@@ -1825,10 +2064,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
>
> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> {
>+ struct irq_domain *domain = chip->irq.domain;
>+
> if (!gpiochip_irqchip_irq_valid(chip, offset))
> return -ENXIO;
>
>- return irq_create_mapping(chip->irq.domain, offset);
>+ if (irq_domain_is_hierarchy(domain)) {
>+ struct irq_fwspec spec;
>+
>+ spec.fwnode = domain->fwnode;
>+ spec.param_count = 2;
>+ spec.param[0] = offset;
>+ spec.param[1] = IRQ_TYPE_NONE;
>+
>+ return irq_create_fwspec_mapping(&spec);
>+ }
>+
>+ return irq_create_mapping(domain, offset);
> }
>
> static int gpiochip_irq_reqres(struct irq_data *d)
>@@ -1905,7 +2157,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> struct lock_class_key *request_key)
> {
> struct irq_chip *irqchip = gpiochip->irq.chip;
>- const struct irq_domain_ops *ops;
>+ const struct irq_domain_ops *ops = NULL;
> struct device_node *np;
> unsigned int type;
> unsigned int i;
>@@ -1941,16 +2193,25 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> gpiochip->irq.lock_key = lock_key;
> gpiochip->irq.request_key = request_key;
>
>- if (gpiochip->irq.domain_ops)
>- ops = gpiochip->irq.domain_ops;
>- else
>- ops = &gpiochip_domain_ops;
>-
>- gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
>- gpiochip->irq.first,
>- ops, gpiochip);
>- if (!gpiochip->irq.domain)
>- return -EINVAL;
>+ /* If a parent irqdomain is provided, let's build a hierarchy */
>+ if (gpiochip_hierarchy_is_hierarchical(gpiochip)) {
>+ int ret = gpiochip_hierarchy_add_domain(gpiochip);
>+ if (ret)
>+ return ret;
>+ } else {
>+ /* Some drivers provide custom irqdomain ops */
>+ if (gpiochip->irq.domain_ops)
>+ ops = gpiochip->irq.domain_ops;
>+
>+ if (!ops)
>+ ops = &gpiochip_domain_ops;
>+ gpiochip->irq.domain = irq_domain_add_simple(np,
>+ gpiochip->ngpio,
>+ gpiochip->irq.first,
>+ ops, gpiochip);
>+ if (!gpiochip->irq.domain)
>+ return -EINVAL;
>+ }
>
> if (gpiochip->irq.parent_handler) {
> void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
>diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>index a1d273c96016..e32d02cb2d08 100644
>--- a/include/linux/gpio/driver.h
>+++ b/include/linux/gpio/driver.h
>@@ -22,6 +22,9 @@ enum gpiod_flags;
> #ifdef CONFIG_GPIOLIB
>
> #ifdef CONFIG_GPIOLIB_IRQCHIP
>+
>+struct gpio_chip;
>+
> /**
> * struct gpio_irq_chip - GPIO interrupt controller
> */
>@@ -48,6 +51,49 @@ struct gpio_irq_chip {
> */
> const struct irq_domain_ops *domain_ops;
>
>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>+ /**
>+ * @fwnode:
>+ *
>+ * Firmware node corresponding to this gpiochip/irqchip, necessary
>+ * for hierarchical irqdomain support.
>+ */
>+ struct fwnode_handle *fwnode;
>+
>+ /**
>+ * @parent_domain:
>+ *
>+ * If non-NULL, will be set as the parent of this GPIO interrupt
>+ * controller's IRQ domain to establish a hierarchical interrupt
>+ * domain. The presence of this will activate the hierarchical
>+ * interrupt support.
>+ */
>+ struct irq_domain *parent_domain;
>+
>+ /**
>+ * @child_to_parent_hwirq:
>+ *
>+ * This callback translates a child hardware IRQ offset to a parent
>+ * hardware IRQ offset on a hierarchical interrupt chip. The child
>+ * hardware IRQs correspond to the GPIO index 0..ngpio-1 (see the
>+ * ngpio field of struct gpio_chip) and the corresponding parent
>+ * hardware IRQ and type (such as IRQ_TYPE_*) shall be returned by
>+ * the driver. The driver can calculate this from an offset or using
>+ * a lookup table or whatever method is best for this chip. Return
>+ * 0 on successful translation in the driver.
>+ *
>+ * If some ranges of hardware IRQs do not have a corresponding parent
>+ * HWIRQ, return -EINVAL, but also make sure to fill in @valid_mask and
>+ * @need_valid_mask to make these GPIO lines unavailable for
>+ * translation.
>+ */
>+ int (*child_to_parent_hwirq)(struct gpio_chip *chip,
>+ unsigned int child_hwirq,
>+ unsigned int child_type,
>+ unsigned int *parent_hwirq,
>+ unsigned int *parent_type);
Would irq_fwspec(s) be better than passing all these arguments around?
Thanks,
Lina
>+#endif
>+
> /**
> * @handler:
> *
>--
>2.21.0
>
^ permalink raw reply
* Re: [PATCH] gpio: siox: Pass irqchip when adding gpiochip
From: Thorsten Scherer @ 2019-06-26 19:36 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Linus Walleij, linux-gpio, Bartosz Golaszewski, Thierry Reding
In-Reply-To: <20190625193328.sxvhastsatc62msh@pengutronix.de>
Hello,
On Tue, Jun 25, 2019 at 09:33:28PM +0200, Uwe Kleine-König wrote:
> Hello Linus,
>
> On Tue, Jun 25, 2019 at 12:53:46PM +0200, Linus Walleij wrote:
> > We need to convert all old gpio irqchips to pass the irqchip
> > setup along when adding the gpio_chip.
> >
> > For chained irqchips this is a pretty straight-forward
> > conversion.
> >
> > The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as
> > default IRQ trigger type which seems wrong, as consumers
> > should explicitly set this up, so set IRQ_TYPE_NONE instead.
> >
> > Also gpiochip_remove() was called on the errorpath if
> > gpiochip_add() failed: this is wrong, if the chip failed
> > to add it is not there so it should not be removed.
>
> So we have a bugfix (gpiochip_remove() in error path), a change of
> default behaviour (IRQ_TYPE_EDGE_RISING -> IRQ_TYPE_NONE) and a cleanup
> for an API change (I'm guessing here) in a single patch. :-|
>
> @Thorsten: I'm not entirely sure if there is code relying on the default
> IRQ_TYPE_EDGE_RISING. Do you know off-hand?
Didn't know off the top of my head. So I dug through some application
code. As far as I can tell, nothing relies on edge rising. But I would
not bet on it. And I don't know about code in the other departments.
>
> Best regards
> Uwe
Best regards
Thorsten
>
> > diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
> > index fb4e318ab028..e5c85dc932e8 100644
> > --- a/drivers/gpio/gpio-siox.c
> > +++ b/drivers/gpio/gpio-siox.c
> > @@ -211,6 +211,7 @@ static int gpio_siox_get_direction(struct gpio_chip *chip, unsigned int offset)
> > static int gpio_siox_probe(struct siox_device *sdevice)
> > {
> > struct gpio_siox_ddata *ddata;
> > + struct gpio_irq_chip *girq;
> > int ret;
> >
> > ddata = devm_kzalloc(&sdevice->dev, sizeof(*ddata), GFP_KERNEL);
> > @@ -239,20 +240,16 @@ static int gpio_siox_probe(struct siox_device *sdevice)
> > ddata->ichip.irq_unmask = gpio_siox_irq_unmask;
> > ddata->ichip.irq_set_type = gpio_siox_irq_set_type;
> >
> > + girq = &ddata->gchip.irq;
> > + girq->chip = &ddata->ichip;
> > + girq->default_type = IRQ_TYPE_NONE;
> > + girq->handler = handle_level_irq;
> > +
> > ret = gpiochip_add(&ddata->gchip);
> > if (ret) {
> > dev_err(&sdevice->dev,
> > "Failed to register gpio chip (%d)\n", ret);
> > - goto err_gpiochip;
> > - }
> > -
> > - ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
> > - 0, handle_level_irq, IRQ_TYPE_EDGE_RISING);
> > - if (ret) {
> > - dev_err(&sdevice->dev,
> > - "Failed to register irq chip (%d)\n", ret);
> > -err_gpiochip:
> > - gpiochip_remove(&ddata->gchip);
> > + return ret;
> > }
> >
> > return ret;
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
--
Thorsten Scherer - Eckelmann AG
https://www.eckelmann.de
^ permalink raw reply
* Re: [RFC] Addition of kernel
From: Enrico Weigelt, metux IT consult @ 2019-06-26 18:16 UTC (permalink / raw)
To: Linus Walleij, Martyn Welch
Cc: Bartosz Golaszewski, open list:GPIO SUBSYSTEM, kernel
In-Reply-To: <CACRpkdaLEDmJ49m_fpuuA1e33hTtyB-LsyZeOmpRybbULgmHDA@mail.gmail.com>
On 24.06.19 23:46, Linus Walleij wrote:
> A GPS unit should be handled using the GNSS subsystem in
> drivers/gnss:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gnss
yes, since serdev we can also easily drive serial-connected devices
(which many gps receivers are) from inside the kernel. there're also
composite devices (eg. combined basebands) which also have gps amongst
other things, so an own subsystem for gps devices is the way to go.
> While we do encourage to use the right subsystems for this kind
> of stuff there are certain cases we do defer to be handled in userspace,
> but not many. These include one-off things like prototypes and
Those are cases which probably nobody wants to have special support
in the mainline kernel ... i recall some rules about no kernel drivers
without corresponding free userland ...
> factory lines with a myriad of relays (some PLC usecases),
> door openers (we don't want drivers/dooropener)
Actually, I've got something like that in the pipeline: a generic relais
subsystem for anything that just switches on/off. Haven't gathered all
requirements yet - for now just abusing LED for that (yes, also actually
door openers). Okay, door openers could be a complex matter on their
own, depending on how it electrically/mechanically works - some devices
let motors spin until an end reached, etc. ... but haven't had an
actual usecase for putting such things into the kernel.
> or fire alarm button
Button -> input subsystem ?
> (but definately any elaborate IIO sensors
> goes into drivers/iio) so it is a bit on case-by-case intuition
> here.
yes, and it's primarily about high level functionality. in industrial
world we often have composite devices that span multiple subsystems.
I any case, for a good decision we need to know what exactly some
individual device actually does.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 01/30] include: linux: platform_device: more helpers for declaring platform drivers
From: Enrico Weigelt, metux IT consult @ 2019-06-26 17:27 UTC (permalink / raw)
To: Uwe Kleine-König, Enrico Weigelt, metux IT consult
Cc: linux-kernel, grygorii.strashko, mcoquelin.stm32, thloh, festevam,
linus.walleij, khilman, patches, bgolaszewski, linux-omap,
linux-gpio, linux-imx, kernel, ssantosh, linux-tegra,
andriy.shevchenko, shawnguo, s.hauer, alexandre.torgue
In-Reply-To: <20190626061405.qsqq5na4oactuo6f@pengutronix.de>
On 26.06.19 08:14, Uwe Kleine-König wrote:
Hi,
> Would it make sense to do something like:
>
> #define __module_platform_driver(__platform_driver, __initlvl) \
> static int __init __platform_driver##_init(void) \
> { \
> return platform_driver_register(&(__platform_driver)); \
> } \
> __initlvl ## _initcall(__platform_driver##_init); \
> static void __exit __platform_driver##_exit(void) \
> { \
> platform_driver_unregister(&(__platform_driver)); \
> } \
> module_exit(__platform_driver##_exit);
>
> #define postcore_platform_driver(__platform_driver)
__module_platform_driver(__platform_driver, postcore)
> #define subsys_platform_driver(__platform_driver)
__module_platform_driver(__platform_driver, subsys)
> ...
>
> Which would be more compact and makes the difference between these
> macros a bit more obvious.
yeah, could do that, but not sure whether it's really good for
readability when we have so many nested macros :p
OTOH, I didn't want to touch the existing macros for now, just trim down
the actual init boilerplate, postponing further compactions for later.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH 1/2] siox: add helper macro to simplify driver registration
From: Enrico Weigelt, metux IT consult @ 2019-06-26 17:31 UTC (permalink / raw)
To: Linus Walleij, Enrico Weigelt, metux IT consult
Cc: linux-kernel@vger.kernel.org, Sascha Hauer, Bartosz Golaszewski,
open list:GPIO SUBSYSTEM, t.scherer
In-Reply-To: <CACRpkdbZ68O41dx7uCccUF_jvmC3_YVvWEEo9igknMC95QEXhQ@mail.gmail.com>
On 25.06.19 11:26, Linus Walleij wrote:
> On Mon, Jun 24, 2019 at 7:40 AM Enrico Weigelt, metux IT consult
> <info@metux.net> wrote:
>
>> From: Enrico Weigelt <info@metux.net>
>>
>> Add more helper macros for trivial driver init cases, similar to the
>> already existing module_platform_driver() or module_i2c_driver().
>>
>> This helps to reduce driver init boilerplate.
>>
>> Signed-off-by: Enrico Weigelt <info@metux.net>
>
> Patch applied with Uwe's ACK.
thanks to both of you :)
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH V4 03/18] gpio: tegra: use resume_noirq for tegra gpio resume
From: Sowjanya Komatineni @ 2019-06-26 17:06 UTC (permalink / raw)
To: Linus Walleij
Cc: thierry.reding@gmail.com, Jon Hunter, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Stefan Agner, Mark Rutland,
Peter De Schrijver, Prashant Gaikwad, Stephen Boyd, linux-clk,
open list:GPIO SUBSYSTEM, jckuo, Joseph Lo, talho, linux-tegra,
linux-kernel@vger.kernel.org, Mikko Perttunen, spatra,
Rob Herring, Dmitry Osipenko,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
In-Reply-To: <CACRpkda0=HeRco8kExdf6TmiLOnCec3Ek06s-MdjNJvVGw3ZNQ@mail.gmail.com>
On 6/25/19 6:38 AM, Linus Walleij wrote:
> On Mon, Jun 24, 2019 at 5:03 AM Sowjanya Komatineni
> <skomatineni@nvidia.com> wrote:
>
>> During SC7 resume, PARKED bit clear from the pinmux registers may
>> cause a glitch on the GPIO lines.
>>
>> So, Tegra GPIOs restore should happen prior to restoring Tegra pinmux
>> to keep the GPIO lines in a known good state prior to clearing PARKED
>> bit.
>>
>> This patch has fix for this by moving Tegra GPIOs restore to happen
>> very early than pinctrl resume.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> Can this patch be applied in isolation from the other patches?
>
> WOuld be nice to have at least Thierry's ACK on it before I
> apply it.
>
> Yours,
> Linus Walleij
Just to confirm, Will not include this in V5 as you are planning to
apply this patch separately.
Thanks
Sowjanya
^ permalink raw reply
* Re: [PATCH V4 12/18] clk: tegra210: support for Tegra210 clocks suspend and resume
From: Sowjanya Komatineni @ 2019-06-26 16:46 UTC (permalink / raw)
To: Thierry Reding
Cc: jonathanh, tglx, jason, marc.zyngier, linus.walleij, stefan,
mark.rutland, pdeschrijver, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, josephl, talho, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <20190626101616.GE6362@ulmo>
On 6/26/19 3:16 AM, Thierry Reding wrote:
> On Sun, Jun 23, 2019 at 08:02:53PM -0700, Sowjanya Komatineni wrote:
>> This patch adds system suspend and resume support for Tegra210
>> clocks.
>>
>> All the CAR controller settings are lost on suspend when core power
>> goes off.
>>
>> This patch has implementation for saving and restoring all the PLLs
>> and clocks context during system suspend and resume to have the
>> clocks back to same state for normal operation.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/clk/tegra/clk-tegra210.c | 115 ++++++++++++++++++++++++++++++++++++++-
>> drivers/clk/tegra/clk.c | 14 +++++
>> drivers/clk/tegra/clk.h | 1 +
>> 3 files changed, 127 insertions(+), 3 deletions(-)
> Can you give a brief overview of how you've been testing this? Is this
> just about confirming that "things work fine" after resume? Could we be
> more methodical and perhaps check the state of the clock tree before
> suspend (perhaps by inspecting debugfs) and comparing it to the state
> after resume?
>
> One minor nit below...
Yes, I tested by checking clock tree for counts and rate through debugfs
clk_rate nodes as well for all clocks before suspend and on resume.
>> diff --git a/drivers/clk/tegra/clk-tegra210.c b/drivers/clk/tegra/clk-tegra210.c
>> index c414cd72d045..4fb3509fe9f6 100644
>> --- a/drivers/clk/tegra/clk-tegra210.c
>> +++ b/drivers/clk/tegra/clk-tegra210.c
>> @@ -9,10 +9,12 @@
>> #include <linux/clkdev.h>
>> #include <linux/of.h>
>> #include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> #include <linux/delay.h>
>> #include <linux/export.h>
>> #include <linux/mutex.h>
>> #include <linux/clk/tegra.h>
>> +#include <linux/syscore_ops.h>
>> #include <dt-bindings/clock/tegra210-car.h>
>> #include <dt-bindings/reset/tegra210-car.h>
>> #include <linux/iopoll.h>
>> @@ -20,6 +22,7 @@
>> #include <soc/tegra/pmc.h>
>>
>> #include "clk.h"
>> +#include "clk-dfll.h"
>> #include "clk-id.h"
>>
>> /*
>> @@ -225,6 +228,7 @@
>>
>> #define CLK_RST_CONTROLLER_RST_DEV_Y_SET 0x2a8
>> #define CLK_RST_CONTROLLER_RST_DEV_Y_CLR 0x2ac
>> +#define CPU_SOFTRST_CTRL 0x380
>>
>> #define LVL2_CLK_GATE_OVRA 0xf8
>> #define LVL2_CLK_GATE_OVRC 0x3a0
>> @@ -2820,6 +2824,7 @@ static int tegra210_enable_pllu(void)
>> struct tegra_clk_pll_freq_table *fentry;
>> struct tegra_clk_pll pllu;
>> u32 reg;
>> + int ret;
>>
>> for (fentry = pll_u_freq_table; fentry->input_rate; fentry++) {
>> if (fentry->input_rate == pll_ref_freq)
>> @@ -2847,10 +2852,10 @@ static int tegra210_enable_pllu(void)
>> fence_udelay(1, clk_base);
>> reg |= PLL_ENABLE;
>> writel(reg, clk_base + PLLU_BASE);
>> + fence_udelay(1, clk_base);
>>
>> - readl_relaxed_poll_timeout_atomic(clk_base + PLLU_BASE, reg,
>> - reg & PLL_BASE_LOCK, 2, 1000);
>> - if (!(reg & PLL_BASE_LOCK)) {
>> + ret = tegra210_wait_for_mask(&pllu, PLLU_BASE, PLL_BASE_LOCK);
>> + if (ret) {
>> pr_err("Timed out waiting for PLL_U to lock\n");
>> return -ETIMEDOUT;
>> }
>> @@ -3282,6 +3287,103 @@ static void tegra210_disable_cpu_clock(u32 cpu)
>> }
>>
>> #ifdef CONFIG_PM_SLEEP
>> +static u32 cpu_softrst_ctx[3];
>> +static struct platform_device *dfll_pdev;
>> +#define car_readl(_base, _off) readl_relaxed(clk_base + (_base) + ((_off) * 4))
>> +#define car_writel(_val, _base, _off) \
>> + writel_relaxed(_val, clk_base + (_base) + ((_off) * 4))
>> +
>> +static int tegra210_clk_suspend(void)
>> +{
>> + int i;
> This can be unsigned int.
>
>> + struct device_node *node;
>> +
>> + tegra_cclkg_burst_policy_save_context();
>> +
>> + if (!dfll_pdev) {
>> + node = of_find_compatible_node(NULL, NULL,
>> + "nvidia,tegra210-dfll");
>> + if (node)
>> + dfll_pdev = of_find_device_by_node(node);
>> +
>> + of_node_put(node);
>> + if (!dfll_pdev)
>> + pr_err("dfll node not found. no suspend for dfll\n");
>> + }
>> +
>> + if (dfll_pdev)
>> + tegra_dfll_suspend(dfll_pdev);
>> +
>> + /* Enable PLLP_OUT_CPU after dfll suspend */
>> + tegra_clk_set_pllp_out_cpu(true);
>> +
>> + tegra_sclk_cclklp_burst_policy_save_context();
>> +
>> + clk_save_context();
>> +
>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>> + cpu_softrst_ctx[i] = car_readl(CPU_SOFTRST_CTRL, i);
>> +
>> + return 0;
>> +}
>> +
>> +static void tegra210_clk_resume(void)
>> +{
>> + int i;
> And here too.
>
> With that:
>
> Acked-by: Thierry Reding <treding@nvidia.com>
>
>> + struct clk_hw *parent;
>> + struct clk *clk;
>> +
>> + /*
>> + * clk_restore_context restores clocks as per the clock tree.
>> + *
>> + * dfllCPU_out is first in the clock tree to get restored and it
>> + * involves programming DFLL controller along with restoring CPUG
>> + * clock burst policy.
>> + *
>> + * DFLL programming needs dfll_ref and dfll_soc peripheral clocks
>> + * to be restores which are part ofthe peripheral clocks.
>> + * So, peripheral clocks restore should happen prior to dfll clock
>> + * restore.
>> + */
>> +
>> + tegra_clk_osc_resume(clk_base);
>> + for (i = 0; i < ARRAY_SIZE(cpu_softrst_ctx); i++)
>> + car_writel(cpu_softrst_ctx[i], CPU_SOFTRST_CTRL, i);
>> +
>> + /* restore all plls and peripheral clocks */
>> + tegra210_init_pllu();
>> + clk_restore_context();
>> +
>> + fence_udelay(5, clk_base);
>> +
>> + /* resume SCLK and CPULP clocks */
>> + tegra_sclk_cpulp_burst_policy_restore_context();
>> +
>> + /*
>> + * restore CPUG clocks:
>> + * - enable DFLL in open loop mode
>> + * - switch CPUG to DFLL clock source
>> + * - close DFLL loop
>> + * - sync PLLX state
>> + */
>> + if (dfll_pdev)
>> + tegra_dfll_resume(dfll_pdev, false);
>> +
>> + tegra_cclkg_burst_policy_restore_context();
>> + fence_udelay(2, clk_base);
>> +
>> + if (dfll_pdev)
>> + tegra_dfll_resume(dfll_pdev, true);
>> +
>> + parent = clk_hw_get_parent(__clk_get_hw(clks[TEGRA210_CLK_CCLK_G]));
>> + clk = clks[TEGRA210_CLK_PLL_X];
>> + if (parent != __clk_get_hw(clk))
>> + tegra_clk_sync_state_pll(__clk_get_hw(clk));
>> +
>> + /* Disable PLL_OUT_CPU after DFLL resume */
>> + tegra_clk_set_pllp_out_cpu(false);
>> +}
>> +
>> static void tegra210_cpu_clock_suspend(void)
>> {
>> /* switch coresite to clk_m, save off original source */
>> @@ -3297,6 +3399,11 @@ static void tegra210_cpu_clock_resume(void)
>> }
>> #endif
>>
>> +static struct syscore_ops tegra_clk_syscore_ops = {
>> + .suspend = tegra210_clk_suspend,
>> + .resume = tegra210_clk_resume,
>> +};
>> +
>> static struct tegra_cpu_car_ops tegra210_cpu_car_ops = {
>> .wait_for_reset = tegra210_wait_cpu_in_reset,
>> .disable_clock = tegra210_disable_cpu_clock,
>> @@ -3582,5 +3689,7 @@ static void __init tegra210_clock_init(struct device_node *np)
>> tegra210_mbist_clk_init();
>>
>> tegra_cpu_car_ops = &tegra210_cpu_car_ops;
>> +
>> + register_syscore_ops(&tegra_clk_syscore_ops);
>> }
>> CLK_OF_DECLARE(tegra210, "nvidia,tegra210-car", tegra210_clock_init);
>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
>> index 9e863362d2bf..96cc9937ea37 100644
>> --- a/drivers/clk/tegra/clk.c
>> +++ b/drivers/clk/tegra/clk.c
>> @@ -23,6 +23,7 @@
>> #define CLK_OUT_ENB_W 0x364
>> #define CLK_OUT_ENB_X 0x280
>> #define CLK_OUT_ENB_Y 0x298
>> +#define CLK_ENB_PLLP_OUT_CPU BIT(31)
>> #define CLK_OUT_ENB_SET_L 0x320
>> #define CLK_OUT_ENB_CLR_L 0x324
>> #define CLK_OUT_ENB_SET_H 0x328
>> @@ -205,6 +206,19 @@ const struct tegra_clk_periph_regs *get_reg_bank(int clkid)
>> }
>> }
>>
>> +void tegra_clk_set_pllp_out_cpu(bool enable)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(clk_base + CLK_OUT_ENB_Y);
>> + if (enable)
>> + val |= CLK_ENB_PLLP_OUT_CPU;
>> + else
>> + val &= ~CLK_ENB_PLLP_OUT_CPU;
>> +
>> + writel_relaxed(val, clk_base + CLK_OUT_ENB_Y);
>> +}
>> +
>> void tegra_cclkg_burst_policy_save_context(void)
>> {
>> unsigned int i;
>> diff --git a/drivers/clk/tegra/clk.h b/drivers/clk/tegra/clk.h
>> index 7b99496fefb9..b994dfaa9602 100644
>> --- a/drivers/clk/tegra/clk.h
>> +++ b/drivers/clk/tegra/clk.h
>> @@ -862,6 +862,7 @@ void tegra_cclkg_burst_policy_restore_context(void);
>> void tegra_sclk_cclklp_burst_policy_save_context(void);
>> void tegra_sclk_cpulp_burst_policy_restore_context(void);
>> void tegra_clk_osc_resume(void __iomem *clk_base);
>> +void tegra_clk_set_pllp_out_cpu(bool enable);
>>
>> /* Combined read fence with delay */
>> #define fence_udelay(delay, reg) \
>> --
>> 2.7.4
>>
^ permalink raw reply
* Re: [PATCH] gpio: tegra: Clean-up debugfs initialisation
From: Bartosz Golaszewski @ 2019-06-26 16:38 UTC (permalink / raw)
To: Jon Hunter
Cc: Linus Walleij, Thierry Reding, linux-gpio, linux-tegra, Greg KH
In-Reply-To: <20190626134258.26991-1-jonathanh@nvidia.com>
śr., 26 cze 2019 o 15:43 Jon Hunter <jonathanh@nvidia.com> napisał(a):
>
> The function tegra_gpio_debuginit() just calls debugfs_create_file()
> and given that there is already a stub function implemented for
> debugfs_create_file() when CONFIG_DEBUG_FS is not enabled, there is
> no need for the function tegra_gpio_debuginit() and so remove it.
>
> Finally, use a space and not a tab between the #ifdef and
> CONFIG_DEBUG_FS.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> drivers/gpio/gpio-tegra.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tegra.c b/drivers/gpio/gpio-tegra.c
> index 0f59161a4701..59b99d8c3647 100644
> --- a/drivers/gpio/gpio-tegra.c
> +++ b/drivers/gpio/gpio-tegra.c
> @@ -508,7 +508,7 @@ static int tegra_gpio_irq_set_wake(struct irq_data *d, unsigned int enable)
> }
> #endif
>
> -#ifdef CONFIG_DEBUG_FS
> +#ifdef CONFIG_DEBUG_FS
>
> #include <linux/debugfs.h>
> #include <linux/seq_file.h>
> @@ -538,19 +538,6 @@ static int tegra_dbg_gpio_show(struct seq_file *s, void *unused)
> }
>
> DEFINE_SHOW_ATTRIBUTE(tegra_dbg_gpio);
> -
> -static void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
> -{
> - debugfs_create_file("tegra_gpio", 0444, NULL, tgi,
> - &tegra_dbg_gpio_fops);
> -}
> -
> -#else
> -
> -static inline void tegra_gpio_debuginit(struct tegra_gpio_info *tgi)
> -{
> -}
> -
> #endif
>
> static const struct dev_pm_ops tegra_gpio_pm_ops = {
> @@ -675,7 +662,8 @@ static int tegra_gpio_probe(struct platform_device *pdev)
> }
> }
>
> - tegra_gpio_debuginit(tgi);
> + debugfs_create_file("tegra_gpio", 0444, NULL, tgi,
> + &tegra_dbg_gpio_fops);
>
> return 0;
> }
> --
> 2.17.1
>
Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
This depends on changes queued by Linus, so I'll let him pick it up directly.
Bart
^ permalink raw reply
* Re: [PATCH V4 18/18] arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings
From: Sowjanya Komatineni @ 2019-06-26 16:28 UTC (permalink / raw)
To: Thierry Reding
Cc: jonathanh, tglx, jason, marc.zyngier, linus.walleij, stefan,
mark.rutland, pdeschrijver, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, josephl, talho, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <20190626102824.GG6362@ulmo>
On 6/26/19 3:28 AM, Thierry Reding wrote:
> On Sun, Jun 23, 2019 at 08:02:59PM -0700, Sowjanya Komatineni wrote:
>> This patch has Jetson TX1 platform specific SC7 timing configuration
>> in device tree.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi | 7 +++++++
>> 1 file changed, 7 insertions(+)
> Can you provide a similar patch for Jetson Nano (p3450-0000)? It'd be
> great if we can make new features like this available across all boards.
>
> Thierry
>
Will add patch for Nano in V5...
>> diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi
>> index 27723829d033..cb58f79deb48 100644
>> --- a/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi
>> +++ b/arch/arm64/boot/dts/nvidia/tegra210-p2180.dtsi
>> @@ -279,6 +279,13 @@
>>
>> pmc@7000e400 {
>> nvidia,invert-interrupt;
>> + nvidia,suspend-mode = <0>;
>> + nvidia,cpu-pwr-good-time = <0>;
>> + nvidia,cpu-pwr-off-time = <0>;
>> + nvidia,core-pwr-good-time = <4587 3876>;
>> + nvidia,core-pwr-off-time = <39065>;
>> + nvidia,core-power-req-active-high;
>> + nvidia,sys-clock-req-active-high;
>> };
>>
>> /* eMMC */
>> --
>> 2.7.4
>>
^ permalink raw reply
* Re: [PATCH V4 14/18] soc/tegra: pmc: add pmc wake support for tegra210
From: Sowjanya Komatineni @ 2019-06-26 16:27 UTC (permalink / raw)
To: Thierry Reding
Cc: jonathanh, tglx, jason, marc.zyngier, linus.walleij, stefan,
mark.rutland, pdeschrijver, pgaikwad, sboyd, linux-clk,
linux-gpio, jckuo, josephl, talho, linux-tegra, linux-kernel,
mperttunen, spatra, robh+dt, digetx, devicetree
In-Reply-To: <20190626102614.GF6362@ulmo>
On 6/26/19 3:26 AM, Thierry Reding wrote:
> On Sun, Jun 23, 2019 at 08:02:55PM -0700, Sowjanya Komatineni wrote:
>> This patch implements PMC wakeup sequence for Tegra210 and defines
>> common used RTC alarm wake event.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>> drivers/soc/tegra/pmc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 111 insertions(+)
> One general note, and it's a really pedantic one, which means that this
> patch is plenty good already: sstart the commit subject with a capital
> letter after the prefix, and watch the capitalization of the rest of the
> line:
>
> soc/tegra: pmc: Add PMC wake support for Tegra210
>
> I will usually fix up these trivialities when applying, but you can save
> me a couple of seconds per patch by doing this right to begin with. =)
>
> Thanks again for the great work on this series!
>
> Thierry
Sorry Thierry. Sure will follow that from now on...
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>> index e87f29a35fcf..603fc3bd73f5 100644
>> --- a/drivers/soc/tegra/pmc.c
>> +++ b/drivers/soc/tegra/pmc.c
>> @@ -57,6 +57,12 @@
>> #define PMC_CNTRL_SYSCLK_OE BIT(11) /* system clock enable */
>> #define PMC_CNTRL_SYSCLK_POLARITY BIT(10) /* sys clk polarity */
>> #define PMC_CNTRL_MAIN_RST BIT(4)
>> +#define PMC_CNTRL_LATCH_WAKEUPS BIT(5)
>> +
>> +#define PMC_WAKE_MASK 0x0c
>> +#define PMC_WAKE_LEVEL 0x10
>> +#define PMC_WAKE_STATUS 0x14
>> +#define PMC_SW_WAKE_STATUS 0x18
>>
>> #define DPD_SAMPLE 0x020
>> #define DPD_SAMPLE_ENABLE BIT(0)
>> @@ -87,6 +93,11 @@
>>
>> #define PMC_SCRATCH41 0x140
>>
>> +#define PMC_WAKE2_MASK 0x160
>> +#define PMC_WAKE2_LEVEL 0x164
>> +#define PMC_WAKE2_STATUS 0x168
>> +#define PMC_SW_WAKE2_STATUS 0x16c
>> +
>> #define PMC_SENSOR_CTRL 0x1b0
>> #define PMC_SENSOR_CTRL_SCRATCH_WRITE BIT(2)
>> #define PMC_SENSOR_CTRL_ENABLE_RST BIT(1)
>> @@ -1921,6 +1932,55 @@ static const struct irq_domain_ops tegra_pmc_irq_domain_ops = {
>> .alloc = tegra_pmc_irq_alloc,
>> };
>>
>> +static int tegra210_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>> +{
>> + struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> + unsigned int offset, bit;
>> + u32 value;
>> +
>> + if (data->hwirq == ULONG_MAX)
>> + return 0;
>> +
>> + offset = data->hwirq / 32;
>> + bit = data->hwirq % 32;
>> +
>> + /*
>> + * latch wakeups to SW_WAKE_STATUS register to capture events
>> + * that would not make it into wakeup event register during LP0 exit.
>> + */
>> + value = tegra_pmc_readl(pmc, PMC_CNTRL);
>> + value |= PMC_CNTRL_LATCH_WAKEUPS;
>> + tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> + udelay(120);
>> +
>> + value &= ~PMC_CNTRL_LATCH_WAKEUPS;
>> + tegra_pmc_writel(pmc, value, PMC_CNTRL);
>> + udelay(120);
>> +
>> + tegra_pmc_writel(pmc, 0, PMC_SW_WAKE_STATUS);
>> + tegra_pmc_writel(pmc, 0, PMC_SW_WAKE2_STATUS);
>> +
>> + tegra_pmc_writel(pmc, 0, PMC_WAKE_STATUS);
>> + tegra_pmc_writel(pmc, 0, PMC_WAKE2_STATUS);
>> +
>> + /* enable PMC wake */
>> + if (data->hwirq >= 32)
>> + offset = PMC_WAKE2_MASK;
>> + else
>> + offset = PMC_WAKE_MASK;
>> +
>> + value = tegra_pmc_readl(pmc, offset);
>> +
>> + if (on)
>> + value |= 1 << bit;
>> + else
>> + value &= ~(1 << bit);
>> +
>> + tegra_pmc_writel(pmc, value, offset);
>> +
>> + return 0;
>> +}
>> +
>> static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>> {
>> struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> @@ -1953,6 +2013,49 @@ static int tegra186_pmc_irq_set_wake(struct irq_data *data, unsigned int on)
>> return 0;
>> }
>>
>> +static int tegra210_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> + struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> + unsigned int offset, bit;
>> + u32 value;
>> +
>> + if (data->hwirq == ULONG_MAX)
>> + return 0;
>> +
>> + offset = data->hwirq / 32;
>> + bit = data->hwirq % 32;
>> +
>> + if (data->hwirq >= 32)
>> + offset = PMC_WAKE2_LEVEL;
>> + else
>> + offset = PMC_WAKE_LEVEL;
>> +
>> + value = tegra_pmc_readl(pmc, offset);
>> +
>> + switch (type) {
>> + case IRQ_TYPE_EDGE_RISING:
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + value |= 1 << bit;
>> + break;
>> +
>> + case IRQ_TYPE_EDGE_FALLING:
>> + case IRQ_TYPE_LEVEL_LOW:
>> + value &= ~(1 << bit);
>> + break;
>> +
>> + case IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING:
>> + value ^= 1 << bit;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +
>> + tegra_pmc_writel(pmc, value, offset);
>> +
>> + return 0;
>> +}
>> +
>> static int tegra186_pmc_irq_set_type(struct irq_data *data, unsigned int type)
>> {
>> struct tegra_pmc *pmc = irq_data_get_irq_chip_data(data);
>> @@ -2541,6 +2644,10 @@ static const struct pinctrl_pin_desc tegra210_pin_descs[] = {
>> TEGRA210_IO_PAD_TABLE(TEGRA_IO_PIN_DESC)
>> };
>>
>> +static const struct tegra_wake_event tegra210_wake_events[] = {
>> + TEGRA_WAKE_IRQ("rtc", 16, 2),
>> +};
>> +
>> static const struct tegra_pmc_soc tegra210_pmc_soc = {
>> .num_powergates = ARRAY_SIZE(tegra210_powergates),
>> .powergates = tegra210_powergates,
>> @@ -2558,10 +2665,14 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>> .regs = &tegra20_pmc_regs,
>> .init = tegra20_pmc_init,
>> .setup_irq_polarity = tegra20_pmc_setup_irq_polarity,
>> + .irq_set_wake = tegra210_pmc_irq_set_wake,
>> + .irq_set_type = tegra210_pmc_irq_set_type,
>> .reset_sources = tegra210_reset_sources,
>> .num_reset_sources = ARRAY_SIZE(tegra210_reset_sources),
>> .reset_levels = NULL,
>> .num_reset_levels = 0,
>> + .num_wake_events = ARRAY_SIZE(tegra210_wake_events),
>> + .wake_events = tegra210_wake_events,
>> };
>>
>> #define TEGRA186_IO_PAD_TABLE(_pad) \
>> --
>> 2.7.4
^ permalink raw reply
* Re: [PATCH v1] arm64: dts: qcom: msm8998: Add i2c5 pins
From: Marc Gonzalez @ 2019-06-26 16:20 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Linus Walleij, Jeffrey Hugo, Andy Gross, MSM, gpio, Rob Herring,
Mark Rutland, I2C
In-Reply-To: <20190502151244.GM2938@tuxbook-pro>
On 02/05/2019 17:12, Bjorn Andersson wrote:
> On Mon 29 Apr 01:38 PDT 2019, Marc Gonzalez wrote:
>
>> On 27/04/2019 06:51, Bjorn Andersson wrote:
>>
>>> On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:
>>>
>>>> Downstream source:
>>>> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>>>> ---
>>>> arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> index 6db70acd38ee..d0a95c70d1e7 100644
>>>> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> @@ -2,6 +2,13 @@
>>>> /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>>>>
>>>> &tlmm {
>>>> + i2c5_default: i2c5_default {
>>>> + pins = "gpio87", "gpio88";
>>>> + function = "blsp_i2c5";
>>>> + drive-strength = <2>;
>>>> + bias-disable;
>>>> + };
>>>
>>> You need to reference this node for it to make a difference.
>>
>> Right. I do have a local board file referencing i2c5_default, which I plan
>> to submit at some point. It contains:
>>
>> &blsp1_i2c5 {
>> status = "ok";
>> clock-frequency = <100000>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&i2c5_default>;
>> };
>>
>>> Also the drive-strength and bias are board specific, so please move this
>>> to your board dts (and reference the node).
>>
>> Wait... Are you saying there should be no drive-strength nor bias definitions
>> inside msm8998-pins.dtsi?
>>
>> $ grep -c 'strength\|bias' arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> 18
>>
>> Why are the SDHC pins different than the I2C pins?
>>
>> i2c5 is "tied" to gpio87 and gpio88. Could my board designer "reassign"
>> these pins to a different HW block? Or is that immutable?
>>
>
> Right, so it makes a lot of sense to have a node in msm8998.dtsi that
> says that if i2c5 is probed then the associated pinmux should be set up.
>
> But the pinconf (drive-strenght, internal vs external bias) are board
> specific, so this part better go in the board.dts.
>
>
> On sdm845 we put a node with pinmux in the platform.dtsi and then in the
> board we extend this node with the electrical properties of the board.
> This works out pretty well, but we haven't gone back and updated the
> older platforms/boards yet.
Wow, I had completely lost track of this thread...
OK, I think what you had in mind is the following:
(Please confirm before I spin a v2)
diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index f09f3e03f708..9cd1f96dc3c8 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -27,6 +27,18 @@
status = "okay";
};
+&blsp1_i2c5 {
+ status = "ok";
+ clock-frequency = <100000>; /*** NOT SURE... This depends on which devices are on the I2C bus? ***/
+ pinctrl-names = "default";
+ pinctrl-0 = <&i2c5_default>;
+};
+
+&i2c5_default {
+ drive-strength = <2>;
+ bias-disable;
+};
+
&qusb2phy {
status = "okay";
diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
index 6db70acd38ee..dad175a52d03 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
@@ -2,6 +2,11 @@
/* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
&tlmm {
+ i2c5_default: i2c5-default {
+ pins = "gpio87", "gpio88";
+ function = "blsp_i2c5";
+ };
+
sdc2_clk_on: sdc2_clk_on {
config {
pins = "sdc2_clk";
Well, except that there don't seem to be any devices on the i2c5 bus
on the mediabox...
# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --
But there are on several on my batfish board:
# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
0 1 2 3 4 5 6 7 8 9 a b c d e f
00: -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- 47 -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- 68 -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --
Can I submit the arch/arm64/boot/dts/qcom/msm8998-pins.dtsi alone?
Regards.
^ permalink raw reply related
* Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together
From: Jonathan Neuschäfer @ 2019-06-26 15:30 UTC (permalink / raw)
To: Marc Gonzalez; +Cc: Bjorn Andersson, Linus Walleij, MSM, gpio, Jeffrey Hugo
In-Reply-To: <57d0644d-164f-58e7-6c07-9608da4233a3@free.fr>
[-- Attachment #1: Type: text/plain, Size: 564 bytes --]
Hi,
On Wed, Jun 26, 2019 at 04:38:58PM +0200, Marc Gonzalez wrote:
> Preamble: Rename tsif1 to tsif0, tsif2 to tsif1.
> Squash tsif0 pins into a single function. Same for tsif1.
"Preamble:"? What does this mean in context?
BTW, the devicetree people (devicetree@vger.kernel.org, Rob Herring
<robh+dt@kernel.org>), seem to be missing from the Cc list; they are at
least printed when I run "scripts/get_maintainer.pl -f
Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt" on
v5.2-rc4.
thanks and greetings,
Jonathan Neuschäfer
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v1] pinctrl: msm8998: Squash TSIF pins together
From: Marc Gonzalez @ 2019-06-26 14:46 UTC (permalink / raw)
To: Jeffrey Hugo; +Cc: Bjorn Andersson, Linus Walleij, MSM, gpio
In-Reply-To: <CAOCk7Nqi6j07TzH0tp8x8NzwqX1CAG8wj55Yvxw2tVC8cXhxmQ@mail.gmail.com>
On 26/06/2019 16:42, Jeffrey Hugo wrote:
> On Wed, Jun 26, 2019 at 8:40 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
>
>> Preamble: Rename tsif1 to tsif0, tsif2 to tsif1.
>> Squash tsif0 pins into a single function. Same for tsif1.
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>> .../bindings/pinctrl/qcom,msm8998-pinctrl.txt | 19 ++---
>> drivers/pinctrl/qcom/pinctrl-msm8998.c | 76 +++++--------------
>> 2 files changed, 24 insertions(+), 71 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt
>> index 00174f08ba1d..47b0f30a39e9 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt
>> +++ b/Documentation/devicetree/bindings/pinctrl/qcom,msm8998-pinctrl.txt
>> @@ -124,9 +124,8 @@ to specify in a pin configuration subnode:
>> qlink_request, qua_mi2s, sd_card, sd_write, sdc40, sdc41,
>> sdc42, sdc43, sdc4_clk, sdc4_cmd, sec_mi2s, sp_cmu,
>> spkr_i2s, ssbi1, ssc_irq, ter_mi2s, tgu_ch0, tgu_ch1,
>> - tsense_pwm1, tsense_pwm2, tsif1_clk, tsif1_data, tsif1_en,
>> - tsif1_error, tsif1_sync, tsif2_clk, tsif2_data, tsif2_en,
>> - tsif2_error, tsif2_sync, uim1_clk, uim1_data, uim1_present,
>> + tsense_pwm1, tsense_pwm2, tsif0, tsif1,
>> + uim1_clk, uim1_data, uim1_present,
>> uim1_reset, uim2_clk, uim2_data, uim2_present, uim2_reset,
>> uim_batt, usb_phy, vfr_1, vsense_clkout, vsense_data0,
>> vsense_data1, vsense_mode, wlan1_adc0, wlan1_adc1,
>> @@ -179,15 +178,9 @@ Example:
>> #interrupt-cells = <2>;
>>
>> uart_console_active: uart_console_active {
>> - mux {
>> - pins = "gpio4", "gpio5";
>> - function = "blsp_uart8_a";
>> - };
>> -
>> - config {
>> - pins = "gpio4", "gpio5";
>> - drive-strength = <2>;
>> - bias-disable;
>> - };
>> + pins = "gpio4", "gpio5";
>> + function = "blsp_uart8_a";
>> + drive-strength = <2>;
>> + bias-disable;
>> };
>
> Whats with these "uart" changes? Modernizing the example? Doesn't
> seem related to the commit text...
Good point. I squashed one commit too many. I'll drop that hunk
in v2. (Yes, I meant to document the syntax recommended by Bjorn.)
Regards.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox