* [PATCH v2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Joel Stanley @ 2019-07-23 2:16 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <a0a8162e-c21b-4b3d-b096-1676c5cc9758@www.fastmail.com>
On Tue, 23 Jul 2019 at 00:40, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Tue, 23 Jul 2019, at 10:04, Vijay Khemka wrote:
> > Adds voltage regulators Infineon pxe1610 devices to Facebook
> > tiogapass platform.
> >
> > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
>
> Acked-by: Andrew Jeffery <andrew@aj.id.au>
Thanks, applied to aspeed's dt-for-5.4.
Cheers,
Joel
^ permalink raw reply
* [PATCH] dt-bindings: hwmon: Add binding for pxe1610
From: Guenter Roeck @ 2019-07-23 1:56 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <6E2B35D8-B538-4C96-B289-27A87ECD74DB@fb.com>
On 7/22/19 5:12 PM, Vijay Khemka wrote:
>
>
> ?On 7/22/19, 1:06 PM, "Guenter Roeck" <groeck7 at gmail.com on behalf of linux@roeck-us.net> wrote:
>
> On Mon, Jul 22, 2019 at 12:24:48PM -0700, Vijay Khemka wrote:
> > Added new DT binding document for Infineon PXE1610 devices.
> >
> > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> > ---
> > .../devicetree/bindings/hwmon/pxe1610.txt | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/hwmon/pxe1610.txt
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/pxe1610.txt b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> > new file mode 100644
> > index 000000000000..635daf4955db
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> > @@ -0,0 +1,15 @@
> > +pxe1610 properties
> > +
> > +Required properties:
> > +- compatible: Must be one of the following:
> > + - "infineon,pxe1610" for pxe1610
> > + - "infineon,pxe1110" for pxe1610
> > + - "infineon,pxm1310" for pxm1310
> > +- reg: I2C address
> > +
> > +Example:
> > +
> > +vr at 48 {
> > + compatible = "infineon,pxe1610";
> > + reg = <0x48>;
> > +};
>
> Wouldn't it be better to add this to
> ./Documentation/devicetree/bindings/trivial-devices.txt ?
> Sure, I didn't know about this file. I will add and send another patch. It is
> Documentation/devicetree/bindings/trivial-devices.yaml. How do I abandon
> this patch or just leave it.
>
When you send v2, just add the device to the trivial-devices file instead
and describe the differences to v1 (ie this patch).
Guenter
^ permalink raw reply
* [PATCH v2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Andrew Jeffery @ 2019-07-23 0:41 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190723003216.2910042-1-vijaykhemka@fb.com>
On Tue, 23 Jul 2019, at 10:04, Vijay Khemka wrote:
> Adds voltage regulators Infineon pxe1610 devices to Facebook
> tiogapass platform.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> In v2: Renamed vr to regulator and fixed some typo in commit message.
>
> .../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index c4521eda787c..e722e9aef907 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -144,6 +144,42 @@
> &i2c5 {
> status = "okay";
> // CPU Voltage regulators
> + regulator at 48 {
> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> + };
> + regulator at 4a {
> + compatible = "infineon,pxe1610";
> + reg = <0x4a>;
> + };
> + regulator at 50 {
> + compatible = "infineon,pxe1610";
> + reg = <0x50>;
> + };
> + regulator at 52 {
> + compatible = "infineon,pxe1610";
> + reg = <0x52>;
> + };
> + regulator at 58 {
> + compatible = "infineon,pxe1610";
> + reg = <0x58>;
> + };
> + regulator at 5a {
> + compatible = "infineon,pxe1610";
> + reg = <0x5a>;
> + };
> + regulator at 68 {
> + compatible = "infineon,pxe1610";
> + reg = <0x68>;
> + };
> + regulator at 70 {
> + compatible = "infineon,pxe1610";
> + reg = <0x70>;
> + };
> + regulator at 72 {
> + compatible = "infineon,pxe1610";
> + reg = <0x72>;
> + };
> };
>
> &i2c6 {
> --
> 2.17.1
>
>
^ permalink raw reply
* [PATCH 1/2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Vijay Khemka @ 2019-07-23 0:33 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <802c5419-08ec-4a0e-8a50-ad4a1bbf7f3a@www.fastmail.com>
?On 7/22/19, 5:12 PM, "Andrew Jeffery" <andrew@aj.id.au> wrote:
Hi Vijay,
A few nitpicks.
On Tue, 23 Jul 2019, at 05:10, Vijay Khemka wrote:
> Addes
Typo: Adds
Ack
> Voltage
Unnecessary capitalisation.
Ack
> regulators Infineon pxe1610 devices to Facebook
> tiogapass platform.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index c4521eda787c..b7783833a58c 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -144,6 +144,42 @@
> &i2c5 {
> status = "okay";
> // CPU Voltage regulators
> + vr at 48 {
The recommended generic name is 'regulator', so e.g. regulator at 48
Ack: Submitted v2 for this patch.
> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> + };
> + vr at 4a {
> + compatible = "infineon,pxe1610";
> + reg = <0x4a>;
> + };
> + vr at 50 {
> + compatible = "infineon,pxe1610";
> + reg = <0x50>;
> + };
> + vr at 52 {
> + compatible = "infineon,pxe1610";
> + reg = <0x52>;
> + };
> + vr at 58 {
> + compatible = "infineon,pxe1610";
> + reg = <0x58>;
> + };
> + vr at 5a {
> + compatible = "infineon,pxe1610";
> + reg = <0x5a>;
> + };
> + vr at 68 {
> + compatible = "infineon,pxe1610";
> + reg = <0x68>;
> + };
> + vr at 70 {
> + compatible = "infineon,pxe1610";
> + reg = <0x70>;
> + };
> + vr at 72 {
> + compatible = "infineon,pxe1610";
> + reg = <0x72>;
> + };
> };
>
> &i2c6 {
> --
> 2.17.1
>
>
^ permalink raw reply
* [PATCH v2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Vijay Khemka @ 2019-07-23 0:32 UTC (permalink / raw)
To: linux-aspeed
Adds voltage regulators Infineon pxe1610 devices to Facebook
tiogapass platform.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
In v2: Renamed vr to regulator and fixed some typo in commit message.
.../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index c4521eda787c..e722e9aef907 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -144,6 +144,42 @@
&i2c5 {
status = "okay";
// CPU Voltage regulators
+ regulator at 48 {
+ compatible = "infineon,pxe1610";
+ reg = <0x48>;
+ };
+ regulator at 4a {
+ compatible = "infineon,pxe1610";
+ reg = <0x4a>;
+ };
+ regulator at 50 {
+ compatible = "infineon,pxe1610";
+ reg = <0x50>;
+ };
+ regulator at 52 {
+ compatible = "infineon,pxe1610";
+ reg = <0x52>;
+ };
+ regulator at 58 {
+ compatible = "infineon,pxe1610";
+ reg = <0x58>;
+ };
+ regulator at 5a {
+ compatible = "infineon,pxe1610";
+ reg = <0x5a>;
+ };
+ regulator at 68 {
+ compatible = "infineon,pxe1610";
+ reg = <0x68>;
+ };
+ regulator at 70 {
+ compatible = "infineon,pxe1610";
+ reg = <0x70>;
+ };
+ regulator at 72 {
+ compatible = "infineon,pxe1610";
+ reg = <0x72>;
+ };
};
&i2c6 {
--
2.17.1
^ permalink raw reply related
* [PATCH] dt-bindings: Add pxe1610 as a trivial device
From: Vijay Khemka @ 2019-07-23 0:20 UTC (permalink / raw)
To: linux-aspeed
The pxe1610 is a voltage regulator from Infineon. It also supports
other VRs pxe1110 and pxm1310 from Infineon.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 2e742d399e87..1be648828a31 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -99,6 +99,8 @@ properties:
# Infineon IR38064 Voltage Regulator
- infineon,ir38064
# Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
+ - infineon,pxe1610
+ # Infineon PXE1610, PXE1110 and PXM1310 Voltage Regulators
- infineon,slb9635tt
# Infineon SLB9645 I2C TPM (new protocol, max 400khz)
- infineon,slb9645tt
--
2.17.1
^ permalink raw reply related
* [PATCH] dt-bindings: hwmon: Add binding for pxe1610
From: Vijay Khemka @ 2019-07-23 0:12 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190722200622.GA20435@roeck-us.net>
?On 7/22/19, 1:06 PM, "Guenter Roeck" <groeck7 at gmail.com on behalf of linux@roeck-us.net> wrote:
On Mon, Jul 22, 2019 at 12:24:48PM -0700, Vijay Khemka wrote:
> Added new DT binding document for Infineon PXE1610 devices.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../devicetree/bindings/hwmon/pxe1610.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pxe1610.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pxe1610.txt b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> new file mode 100644
> index 000000000000..635daf4955db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> @@ -0,0 +1,15 @@
> +pxe1610 properties
> +
> +Required properties:
> +- compatible: Must be one of the following:
> + - "infineon,pxe1610" for pxe1610
> + - "infineon,pxe1110" for pxe1610
> + - "infineon,pxm1310" for pxm1310
> +- reg: I2C address
> +
> +Example:
> +
> +vr at 48 {
> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> +};
Wouldn't it be better to add this to
./Documentation/devicetree/bindings/trivial-devices.txt ?
Sure, I didn't know about this file. I will add and send another patch. It is
Documentation/devicetree/bindings/trivial-devices.yaml. How do I abandon
this patch or just leave it.
Thanks,
Guenter
^ permalink raw reply
* [PATCH 1/2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Andrew Jeffery @ 2019-07-23 0:11 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190722192451.1947348-1-vijaykhemka@fb.com>
Hi Vijay,
A few nitpicks.
On Tue, 23 Jul 2019, at 05:10, Vijay Khemka wrote:
> Addes
Typo: Adds
> Voltage
Unnecessary capitalisation.
> regulators Infineon pxe1610 devices to Facebook
> tiogapass platform.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> index c4521eda787c..b7783833a58c 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
> @@ -144,6 +144,42 @@
> &i2c5 {
> status = "okay";
> // CPU Voltage regulators
> + vr at 48 {
The recommended generic name is 'regulator', so e.g. regulator at 48
> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> + };
> + vr at 4a {
> + compatible = "infineon,pxe1610";
> + reg = <0x4a>;
> + };
> + vr at 50 {
> + compatible = "infineon,pxe1610";
> + reg = <0x50>;
> + };
> + vr at 52 {
> + compatible = "infineon,pxe1610";
> + reg = <0x52>;
> + };
> + vr at 58 {
> + compatible = "infineon,pxe1610";
> + reg = <0x58>;
> + };
> + vr at 5a {
> + compatible = "infineon,pxe1610";
> + reg = <0x5a>;
> + };
> + vr at 68 {
> + compatible = "infineon,pxe1610";
> + reg = <0x68>;
> + };
> + vr at 70 {
> + compatible = "infineon,pxe1610";
> + reg = <0x70>;
> + };
> + vr at 72 {
> + compatible = "infineon,pxe1610";
> + reg = <0x72>;
> + };
> };
>
> &i2c6 {
> --
> 2.17.1
>
>
^ permalink raw reply
* [PATCH v4 1/8] dt-bindings: soc: Add Aspeed XDMA engine binding documentation
From: Rob Herring @ 2019-07-22 22:43 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1562010839-1113-2-git-send-email-eajames@linux.ibm.com>
On Mon, 1 Jul 2019 14:53:52 -0500, Eddie James wrote:
> Document the bindings.
>
> Reviewed-by: Andrew Jeffrey <andrew@aj.id.au>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> .../devicetree/bindings/soc/aspeed/xdma.txt | 23 ++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/aspeed/xdma.txt
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [v5 2/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-22 20:36 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563564291-9692-3-git-send-email-hongweiz@ami.com>
Hello Linus,
Thanks for your reviewing, please find my inline comment on why we group the
("A", "B", "C", "D") etc. together at below. We will address other concerns
separately.
--Hongwei
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: Saturday, July 20, 2019 3:37 AM
> To: Hongwei Zhang
> Cc: Andrew Jeffery; open list:GPIO SUBSYSTEM; Joel Stanley; linux-aspeed at lists.ozlabs.org; Bartosz
> Golaszewski; linux-kernel at vger.kernel.org; Linux ARM
> Subject: Re: [v5 2/2] gpio: aspeed: Add SGPIO driver
>
> Hi Hongwei,
>
> thanks for your patch!
>
> some comments and nitpicking below:
>
> On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
>
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
>
> > +// SPDX-License-Identifier: GPL-2.0+
>
> I think the SPDX people prefer GPL-2.0-or-later
>
> > +#include <linux/gpio.h>
>
> Do not include this header in any new code using or providing GPIOs.
>
> > +#include <linux/gpio/driver.h>
>
> This should be enough.
>
> > +/*
> > + * Note: The "value" register returns the input value when the GPIO is
> > + * configured as an input.
> > + *
> > + * The "rdata" register returns the output value when the GPIO is
> > + * configured as an output.
> > + */
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > + {
> > + .val_regs = 0x0000,
> > + .rdata_reg = 0x0070,
> > + .irq_regs = 0x0004,
> > + .names = { "A", "B", "C", "D" },
> > + },
> > + {
> > + .val_regs = 0x001C,
> > + .rdata_reg = 0x0074,
> > + .irq_regs = 0x0020,
> > + .names = { "E", "F", "G", "H" },
> > + },
> > + {
> > + .val_regs = 0x0038,
> > + .rdata_reg = 0x0078,
> > + .irq_regs = 0x003C,
> > + .names = { "I", "J" },
> > + },
> > +};
>
> I guess you have been over the reasons why this is one big GPIO chip instead of 10 individual gpio_chips?
>
> It is usally better to have the individual chips, because it is easier to just cut down the code to handle
> one instance and not having to offset around the different address ranges.
>
> Even if they all have the same clock, the clocks are reference counted so it will just be referenced 10
> times at most.
>
> If they share a few common registers it is not good to split it though. So there may be a compelling
> argument for keeping them all together.
>
As you suspected it correctly, AST2500 utilizes all the 32 bits of the registers
(data value, interrupt, etc...), such that using 8-bit bands
[7:0]/[15:8]/23:16]/[31:24] of GPIO_200H for SGPIO_A/B/C/D .
so registering 10 gpiochip drivers separately will make code more
complicated, for example gpio_200 register (data_value reg) has to be
shared by 4 gpiochip instances, and the same is true for gpio204 (interrupt reg),
and other more registers.
So we would prefer to keeping current implementation.
> > +/* This will be resolved at compile time */
>
> I don't see why that matters.
>
> > +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> > + const struct aspeed_sgpio_bank *bank,
> > + const enum aspeed_sgpio_reg reg)
>
> You don't need inline. The compiler will inline it anyway if it see the need for it.
>
> The only time we really use inline is in header files, where we want to point out that this function will be
> inlined as there is no compiled code in header files.
>
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
>
> OK seems fairly standard.
>
> > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int
> > +offset) static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned
> > +int offset, int val) static int aspeed_sgpio_dir_in(struct gpio_chip
> > +*gc, unsigned int offset)
>
> These are fairly standard.
>
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > +offset, int val) {
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
>
> There is a bug here. You fail to write the "val" to the output line, which is the expected semantic of this
> call.
>
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > +int offset)
>
> These are all very simple MMIO accessors.
>
> If you made one gpio_chip per bank, you could just use gpio-mmio.c to control the lines by
>
> select GPIO_GENERIC
>
> ret = bgpio_init(chip, dev, 4,
> base + GPIO_VAL_VALUE ,
> NULL,
> NULL,
> NULL,
> NULL,
> 0);
>
> The MMIO gpio library takes care of shadowing the direction and all.
> It also will implement get/set_multiple() for you for free.
>
> So seriously consider making one gpio_chip per bank.
>
> > +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d) static void
> > +aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) static void
> > +aspeed_sgpio_irq_mask(struct irq_data *d) static void
> > +aspeed_sgpio_irq_unmask(struct irq_data *d) static int
> > +aspeed_sgpio_set_type(struct irq_data *d, unsigned int type) static
> > +void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > + unsigned int i, p, girq;
> > + unsigned long reg;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + const struct aspeed_sgpio_bank *bank =
> > + &aspeed_sgpio_banks[i];
> > +
> > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > +
> > + for_each_set_bit(p, ®, 32) {
> > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > + generic_handle_irq(girq);
> > + }
> > +
> > + }
>
> This also gets really complex with one driver for all the banks.
>
> > + /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
>
> (...)
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > + struct aspeed_sgpio *gpio;
> > + u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> > + int rc;
> > + unsigned long apb_freq;
> > +
> > + /* initialize allocated memory with zeros */
>
> No need for this comment, developers know what "kzalloc" means.
>
> > + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
> > + if (rc < 0) {
> > + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> > + return -EINVAL;
> > + }
> > +
> > + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(gpio->pclk)) {
> > + dev_err(&pdev->dev, "devm_clk_get failed\n");
> > + return PTR_ERR(gpio->pclk);
> > + }
> > +
> > + apb_freq = clk_get_rate(gpio->pclk);
> > +
> > + /*
> > + * From the datasheet,
> > + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> > + * period = 2 * (GPIO254[31:16] + 1) / PCLK
> > + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> > + * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> > + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> > + * GPIO254[31:16] = PCLK / (frequency * 2) - 1
> > + */
> > + if (sgpio_freq == 0)
> > + return -EINVAL;
> > +
> > + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> > +
> > + if (sgpio_clk_div > (1 << 16) - 1)
> > + return -EINVAL;
> > +
> > + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> > + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> > + ASPEED_SGPIO_ENABLE,
> > + gpio->base + ASPEED_SGPIO_CTRL);
>
> This is a separate clock driver.
>
> Break this out as a separate clk device that the other GPIOs grab.
>
> Put this in drivers/clk/clk-aspeed-gpio.c or wherever appropriate with some reg = <0xnnnnnn54 4>;
>
> Then let the GPIO driver grab this clock. This makes it possible to use a per-gpio-bank split of the GPIO
> chips.
>
> It looks a bit complicated but this will work so much better because the clock code is in the clock
> subsystem and the GPIO is split up and becomes a very small driver since it can use gpio MMIO.
>
> Yours,
> Linus Walleij
^ permalink raw reply
* [PATCH linux dev-5.2] ARM: dts: aspeed: swift: Fix FSI GPIOs
From: Olof Johansson @ 2019-07-22 20:23 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190719203037.11795-1-mspinler@linux.ibm.com>
On Fri, Jul 19, 2019 at 1:31 PM Matt Spinler <mspinler@linux.ibm.com> wrote:
>
> From: Matt Spinler <spinler@us.ibm.com>
>
> Change the FSI clock and data GPIOs to match what the hardware turned
> out to use.
>
> Signed-off-by: Matt Spinler <spinler@us.ibm.com>
Hi,
This is not a comment on this patch specifically, just an overall
request for people to adjust how they post patches:
When running get_maintainer.pl on
arch/arm/boot/dts/aspeed-bmc-opp-swift.dts, I get the following:
[...]
linux-arm-kernel at lists.infradead.org (moderated list:ARM/ASPEED MACHINE SUPPORT)
linux-aspeed at lists.ozlabs.org (moderated list:ARM/ASPEED MACHINE SUPPORT)
[...]
Please include both of them on patches. Lots of people aren't
subscribed to the aspeed-only mailing list, so coming back later to
reply to a patch is hard when it hasn't made it into your mailboxes.
Thanks!
-Olof
^ permalink raw reply
* [PATCH] dt-bindings: hwmon: Add binding for pxe1610
From: Guenter Roeck @ 2019-07-22 20:06 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190722192451.1947348-2-vijaykhemka@fb.com>
On Mon, Jul 22, 2019 at 12:24:48PM -0700, Vijay Khemka wrote:
> Added new DT binding document for Infineon PXE1610 devices.
>
> Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
> ---
> .../devicetree/bindings/hwmon/pxe1610.txt | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/pxe1610.txt
>
> diff --git a/Documentation/devicetree/bindings/hwmon/pxe1610.txt b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> new file mode 100644
> index 000000000000..635daf4955db
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
> @@ -0,0 +1,15 @@
> +pxe1610 properties
> +
> +Required properties:
> +- compatible: Must be one of the following:
> + - "infineon,pxe1610" for pxe1610
> + - "infineon,pxe1110" for pxe1610
> + - "infineon,pxm1310" for pxm1310
> +- reg: I2C address
> +
> +Example:
> +
> +vr at 48 {
> + compatible = "infineon,pxe1610";
> + reg = <0x48>;
> +};
Wouldn't it be better to add this to
./Documentation/devicetree/bindings/trivial-devices.txt ?
Thanks,
Guenter
^ permalink raw reply
* [PATCH 2/2] ARM: dts: aspeed: tiogapass: Add Riser card
From: Vijay Khemka @ 2019-07-22 19:24 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190722192451.1947348-1-vijaykhemka@fb.com>
Added i2c mux for riser card and multiple ava card and its sensor
components for Facebook Tiogapass platform
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 230 ++++++++++++++++++
1 file changed, 230 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index b7783833a58c..8d0bcb3cd419 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -12,6 +12,27 @@
aliases {
serial0 = &uart1;
serial4 = &uart5;
+
+ /*
+ * Hardcode the bus number of i2c switches' channels to
+ * avoid breaking the legacy applications.
+ */
+ i2c16 = &imux16;
+ i2c17 = &imux17;
+ i2c18 = &imux18;
+ i2c19 = &imux19;
+ i2c20 = &imux20;
+ i2c21 = &imux21;
+ i2c22 = &imux22;
+ i2c23 = &imux23;
+ i2c24 = &imux24;
+ i2c25 = &imux25;
+ i2c26 = &imux26;
+ i2c27 = &imux27;
+ i2c28 = &imux28;
+ i2c29 = &imux29;
+ i2c30 = &imux30;
+ i2c31 = &imux31;
};
chosen {
stdout-path = &uart5;
@@ -124,6 +145,215 @@
&i2c1 {
status = "okay";
//X24 Riser
+ i2c-switch at 71 {
+ compatible = "nxp,pca9544";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x71>;
+
+ imux16: i2c at 0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+
+ ina219 at 45 {
+ compatible = "ti,ina219";
+ reg = <0x45>;
+ };
+
+ tmp75 at 48 {
+ compatible = "ti,tmp75";
+ reg = <0x48>;
+ };
+
+ tmp421 at 49 {
+ compatible = "ti,tmp75";
+ reg = <0x49>;
+ };
+
+ eeprom at 50 {
+ compatible = "atmel,24c64";
+ reg = <0x50>;
+ pagesize = <32>;
+ };
+
+ i2c-switch at 73 {
+ compatible = "nxp,pca9546";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x73>;
+
+ imux20: i2c at 0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ };
+
+ imux21: i2c at 1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ imux22: i2c at 2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+
+ imux23: i2c at 3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+
+ };
+
+ };
+
+ imux17: i2c at 1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ ina219 at 45 {
+ compatible = "ti,ina219";
+ reg = <0x45>;
+ };
+
+ tmp421 at 48 {
+ compatible = "ti,tmp75";
+ reg = <0x48>;
+ };
+
+ tmp421 at 49 {
+ compatible = "ti,tmp75";
+ reg = <0x49>;
+ };
+
+ eeprom at 50 {
+ compatible = "atmel,24c64";
+ reg = <0x50>;
+ pagesize = <32>;
+ };
+
+ i2c-switch at 73 {
+ compatible = "nxp,pca9546";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x73>;
+
+ imux24: i2c at 0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ };
+
+ imux25: i2c at 1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ imux26: i2c at 2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+
+ imux27: i2c at 3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+
+ };
+
+ };
+
+ imux18: i2c at 2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+
+ ina219 at 45 {
+ compatible = "ti,ina219";
+ reg = <0x45>;
+ };
+
+ tmp421 at 48 {
+ compatible = "ti,tmp75";
+ reg = <0x48>;
+ };
+
+ tmp421 at 49 {
+ compatible = "ti,tmp75";
+ reg = <0x49>;
+ };
+
+ eeprom at 50 {
+ compatible = "atmel,24c64";
+ reg = <0x50>;
+ pagesize = <32>;
+ };
+
+ i2c-switch at 73 {
+ compatible = "nxp,pca9546";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0x73>;
+
+ imux28: i2c at 0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <0>;
+ };
+
+ imux29: i2c at 1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+ };
+
+ imux30: i2c at 2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <2>;
+ };
+
+ imux31: i2c at 3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+ };
+
+ };
+
+ };
+
+ imux19: i2c at 3 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <3>;
+
+ i2c-switch at 40 {
+ compatible = "ti,ina219";
+ reg = <0x40>;
+ };
+
+ i2c-switch at 41 {
+ compatible = "ti,ina219";
+ reg = <0x41>;
+ };
+
+ i2c-switch at 45 {
+ compatible = "ti,ina219";
+ reg = <0x45>;
+ };
+
+ };
+
+ };
};
&i2c2 {
--
2.17.1
^ permalink raw reply related
* [PATCH] dt-bindings: hwmon: Add binding for pxe1610
From: Vijay Khemka @ 2019-07-22 19:24 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <20190722192451.1947348-1-vijaykhemka@fb.com>
Added new DT binding document for Infineon PXE1610 devices.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../devicetree/bindings/hwmon/pxe1610.txt | 15 +++++++++++++++
1 file changed, 15 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/pxe1610.txt
diff --git a/Documentation/devicetree/bindings/hwmon/pxe1610.txt b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
new file mode 100644
index 000000000000..635daf4955db
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pxe1610.txt
@@ -0,0 +1,15 @@
+pxe1610 properties
+
+Required properties:
+- compatible: Must be one of the following:
+ - "infineon,pxe1610" for pxe1610
+ - "infineon,pxe1110" for pxe1610
+ - "infineon,pxm1310" for pxm1310
+- reg: I2C address
+
+Example:
+
+vr at 48 {
+ compatible = "infineon,pxe1610";
+ reg = <0x48>;
+};
--
2.17.1
^ permalink raw reply related
* [PATCH 1/2] ARM: dts: aspeed: tiogapass: Add VR devices
From: Vijay Khemka @ 2019-07-22 19:24 UTC (permalink / raw)
To: linux-aspeed
Addes Voltage regulators Infineon pxe1610 devices to Facebook
tiogapass platform.
Signed-off-by: Vijay Khemka <vijaykhemka@fb.com>
---
.../dts/aspeed-bmc-facebook-tiogapass.dts | 36 +++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
index c4521eda787c..b7783833a58c 100644
--- a/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-facebook-tiogapass.dts
@@ -144,6 +144,42 @@
&i2c5 {
status = "okay";
// CPU Voltage regulators
+ vr at 48 {
+ compatible = "infineon,pxe1610";
+ reg = <0x48>;
+ };
+ vr at 4a {
+ compatible = "infineon,pxe1610";
+ reg = <0x4a>;
+ };
+ vr at 50 {
+ compatible = "infineon,pxe1610";
+ reg = <0x50>;
+ };
+ vr at 52 {
+ compatible = "infineon,pxe1610";
+ reg = <0x52>;
+ };
+ vr at 58 {
+ compatible = "infineon,pxe1610";
+ reg = <0x58>;
+ };
+ vr at 5a {
+ compatible = "infineon,pxe1610";
+ reg = <0x5a>;
+ };
+ vr at 68 {
+ compatible = "infineon,pxe1610";
+ reg = <0x68>;
+ };
+ vr at 70 {
+ compatible = "infineon,pxe1610";
+ reg = <0x70>;
+ };
+ vr at 72 {
+ compatible = "infineon,pxe1610";
+ reg = <0x72>;
+ };
};
&i2c6 {
--
2.17.1
^ permalink raw reply related
* [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Andrew Jeffery @ 2019-07-22 1:42 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <CACRpkdYhVoP75ZDfASW+DH5yf-a5diitiXsh7eLsJx5hsTC9sQ@mail.gmail.com>
On Sat, 20 Jul 2019, at 17:43, Linus Walleij wrote:
> Hi Hongwei,
>
> after looking close at the driver and bindings I have this feeback:
>
> On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
>
> +- reg : Address and length of the register set for the device
>
> This 0x100 range may look simple but in the driver it looks like
> this:
>
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> + {
> + .val_regs = 0x0000,
> + .rdata_reg = 0x0070,
> + .irq_regs = 0x0004,
> + .names = { "A", "B", "C", "D" },
> + },
> + {
> + .val_regs = 0x001C,
> + .rdata_reg = 0x0074,
> + .irq_regs = 0x0020,
> + .names = { "E", "F", "G", "H" },
> + },
> + {
> + .val_regs = 0x0038,
> + .rdata_reg = 0x0078,
> + .irq_regs = 0x003C,
> + .names = { "I", "J" },
> + },
> +};
>
> So first break this into up to 10 different instances with one device
> per bank instead.
>
> For each device:
>
> reg = <0x1e780200 4>, <x1e780204 ..>, <0x1e780270 ..>;
> reg-names = "val", "irq", "rdata";
>
> This way you use the device tree features for locating the
> register offsets instead of hard-coding it into the driver,
> which will make the driver simpler.
>
> > +- ngpios : number of GPIO pins to serialise.
> > + (should be multiple of 8, up to 80 pins)
>
> This is wrong. This should be split into 10 different devices providing
> 8 GPIO lines each. The "ngpios" is not intended to subdivide
> banks, but for the case where you use say bits 0..11 of 32 bits in
> a register by setting ngpios to 12.
>
> I see that they use the same clock divider and the same interrupt,
> but this is better to partition into a separate clock divider driver
> and up to 10 instances of GPIO devices with 8 GPIOs each.
>
> I also see that they use the same interrupt. This is fine, in your
> driver just grab a shared IRQ and all the IRQ handlers will be
> traversed. This is a well known pattern for all operating systems.
>
> > +- clocks : A phandle to the APB clock for SGPM clock division
> > +
> > +- bus-frequency : SGPM CLK frequency
>
> I see that there is a common clock control register in offset 0x54.
>
> Split this out to a separate clock driver that provides a divided clock
> that all GPIO blocks can get, no matter if you use 1, 2 .. 10 of these
> blocks.
>
> The fact that these GPIO banks and the clock register is in the same
> memory range does not really matter. Split up the memory range in
> on reg = per GPIO chip and one reg for the clock.
The issue that I see with this is that the SGPIO bus clock configuration
is not the only field in the SGPIO control register - it also contains the
number of GPIOs the bus should output along with a bus enable bit.
The clock, and particularly the number of GPIOs (due to the nature
of the SGPIO bus) both need to be configured before we set the
enable bit (or obviously all can be done simultaneously).
I'll explore some of the options below (it's a bit train-of-thought,
hopefully it makes sense):
If the clock driver owns the control register, it also needs to know how
many GPIOs we want to emit on the bus. This seems like an awkward
configuration parameter for a clock driver.
To avoid the weird parameter we could protect the control register
with a lock shared between the clock driver and the SGPIO driver. This
way the SGPIO driver could have the ngpios parameter, and request
the clock after its written the ngpios value to the control register. A
regmap would be useful here to avoid the resource clash and it also
provides the required lock.
However, you're also going down the path of splitting the driver such
that there's one instance per bank. With this approach we need to
solve two problems: Accounting for the total number of GPIOs, and
only enabling the bus after the last bank has had its driver probed.
The accounting might be handled by accumulating the number of
GPIOs in each bank in the control register itself, e.g. the driver
implementation does something like:
spin_lock(...)
ctrl = ioread32(...)
ngpios = FIELD_GET(ASPEED_SGPIO_CTRL_NGPIOS, ctrl);
ngpios += 8;
ctrl &= ~ASPEED_SGPIO_CTRL_NGPIOS;
ctrl |= FIELD_PREP(ASPEED_SGPIO_CTRL_NGPIOS, ngpios);
iowrite32(ctrl, ...);
spin_unlock(...);
This works on cold boot of the device when the ngpios field is set to
zero due to reset, however will fail on subsequent warm reboots if
the GPIO IP block is protected from reset by the SoC's watchdog
configuration: the field will not be zeroed in this case, and the
value of the field is represented by `NR_BOOTS * NR_GPIOS`,
which is incorrect. To do this correctly I guess we would need some
other global state held in the driver implementation (zeroed when
the kernel is loaded), and write the incremented value to the control
register on each probe() invocation.
However, that aside, we can't simply enable the bus in the clock
enable callback if enable is called per-bank, as it is called once on
the first request with further requests simply refcounted as you
mentioned. This is exactly the behaviour we can't tolerate with the
bus: it must only be enabled after the last GPIO bank is registered,
when we know the total number of GPIOs to emit. So we can only
call clk_prepare_enable() after the last bank is probed, or in the
probe() of the last bank if the driver has some way to tell it is the
last instance. In my mind per-bank clock management makes the
implementation more involved than it should be, and isn't really
reflective of what the clk subsystem is trying to achieve (violates
clk_prepare()/clk_enable() post conditions, or boils down to just
calling devm_clk_get() in probe() with no further action, which
seems kinda pointless).
Having a separate clk implementation whose instance is requested
once by a monolithic SGPIO driver matches how the hardware is
designed. However, given the issue of sharing the register with the
GPIO count, in my opinion it seems to make more sense to leave the
driver design as is and minimise the accidental complexity
introduced by the behavioural mismatch of the clk abstraction
and the addition of locking where it wasn't previously necessary.
Interested in your thoughts.
Andrew
^ permalink raw reply
* [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Linus Walleij @ 2019-07-20 8:12 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563564291-9692-2-git-send-email-hongweiz@ami.com>
Hi Hongwei,
after looking close at the driver and bindings I have this feeback:
On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
+- reg : Address and length of the register set for the device
This 0x100 range may look simple but in the driver it looks like
this:
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
So first break this into up to 10 different instances with one device
per bank instead.
For each device:
reg = <0x1e780200 4>, <x1e780204 ..>, <0x1e780270 ..>;
reg-names = "val", "irq", "rdata";
This way you use the device tree features for locating the
register offsets instead of hard-coding it into the driver,
which will make the driver simpler.
> +- ngpios : number of GPIO pins to serialise.
> + (should be multiple of 8, up to 80 pins)
This is wrong. This should be split into 10 different devices providing
8 GPIO lines each. The "ngpios" is not intended to subdivide
banks, but for the case where you use say bits 0..11 of 32 bits in
a register by setting ngpios to 12.
I see that they use the same clock divider and the same interrupt,
but this is better to partition into a separate clock divider driver
and up to 10 instances of GPIO devices with 8 GPIOs each.
I also see that they use the same interrupt. This is fine, in your
driver just grab a shared IRQ and all the IRQ handlers will be
traversed. This is a well known pattern for all operating systems.
> +- clocks : A phandle to the APB clock for SGPM clock division
> +
> +- bus-frequency : SGPM CLK frequency
I see that there is a common clock control register in offset 0x54.
Split this out to a separate clock driver that provides a divided clock
that all GPIO blocks can get, no matter if you use 1, 2 .. 10 of these
blocks.
The fact that these GPIO banks and the clock register is in the same
memory range does not really matter. Split up the memory range in
on reg = per GPIO chip and one reg for the clock.
> + Example:
> + sgpio: sgpio at 1e780200 {
> + #gpio-cells = <2>;
> + compatible = "aspeed,ast2500-sgpio";
> + gpio-controller;
> + interrupts = <40>;
> + reg = <0x1e780200 0x0100>;
> + clocks = <&syscon ASPEED_CLK_APB>;
> + interrupt-controller;
> + ngpios = <8>;
> + bus-frequency = <12000000>;
> + };
Splitting this up into a clock controller and 1-10 instances of sgpio
will make things simpler, because it will closer reflect what the hardware
people are doing: they have just created 10 instances of the same
block, and added a clock divider.
You can put the 1-10 instances and the clock divider inside a collected
node "simple-bus" in the device tree:
{
compatible = "simple-bus";
sgpio0: sgpio {
compatible = "aspeed,ast2500-sgpio";
reg = <0x1e780200 ...> ...;
clk = <&sgpioclk>;
};
sgpio1: sgpio {
...
};
...
sgpioclk: clock {
compatible = "aspeed,sgpio-clock";
reg = 0x1e780254 4>;
};
};
This is a better fit with the actual hardware and will make it much
easier to write drivers.
Admittedly DT device partitioning of SoC devices is a grey area,
but here I think the DT infrastructure helps you to break things
down and make it easier, I am thinking in a divide-and-conquer
way about it.
Yours,
Linus Walleij
^ permalink raw reply
* [v5 2/2] gpio: aspeed: Add SGPIO driver
From: Linus Walleij @ 2019-07-20 7:36 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563564291-9692-3-git-send-email-hongweiz@ami.com>
Hi Hongwei,
thanks for your patch!
some comments and nitpicking below:
On Fri, Jul 19, 2019 at 9:25 PM Hongwei Zhang <hongweiz@ami.com> wrote:
> Add SGPIO driver support for Aspeed AST2500 SoC.
>
> Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> +// SPDX-License-Identifier: GPL-2.0+
I think the SPDX people prefer GPL-2.0-or-later
> +#include <linux/gpio.h>
Do not include this header in any new code using or
providing GPIOs.
> +#include <linux/gpio/driver.h>
This should be enough.
> +/*
> + * Note: The "value" register returns the input value when the GPIO is
> + * configured as an input.
> + *
> + * The "rdata" register returns the output value when the GPIO is
> + * configured as an output.
> + */
> +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> + {
> + .val_regs = 0x0000,
> + .rdata_reg = 0x0070,
> + .irq_regs = 0x0004,
> + .names = { "A", "B", "C", "D" },
> + },
> + {
> + .val_regs = 0x001C,
> + .rdata_reg = 0x0074,
> + .irq_regs = 0x0020,
> + .names = { "E", "F", "G", "H" },
> + },
> + {
> + .val_regs = 0x0038,
> + .rdata_reg = 0x0078,
> + .irq_regs = 0x003C,
> + .names = { "I", "J" },
> + },
> +};
I guess you have been over the reasons why this is one big GPIO
chip instead of 10 individual gpio_chips?
It is usally better to have the individual chips, because it is easier
to just cut down the code to handle one instance and not having
to offset around the different address ranges.
Even if they all have the same clock, the clocks are reference
counted so it will just be referenced 10 times at most.
If they share a few common registers it is not good to split it
though. So there may be a compelling argument for keeping them
all together.
> +/* This will be resolved at compile time */
I don't see why that matters.
> +static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
> + const struct aspeed_sgpio_bank *bank,
> + const enum aspeed_sgpio_reg reg)
You don't need inline. The compiler will inline it anyway if it
see the need for it.
The only time we really use inline is in header files, where we
want to point out that this function will be inlined as there is no
compiled code in header files.
> +#define GPIO_BANK(x) ((x) >> 5)
> +#define GPIO_OFFSET(x) ((x) & 0x1f)
> +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
OK seems fairly standard.
> +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
> +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
> +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
These are fairly standard.
> +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
> +{
> + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> + unsigned long flags;
> +
> + spin_lock_irqsave(&gpio->lock, flags);
> + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> + spin_unlock_irqrestore(&gpio->lock, flags);
> +
> + return 0;
> +}
There is a bug here. You fail to write the "val" to the output
line, which is the expected semantic of this call.
> +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
These are all very simple MMIO accessors.
If you made one gpio_chip per bank, you could just use gpio-mmio.c
to control the lines by
select GPIO_GENERIC
ret = bgpio_init(chip, dev, 4,
base + GPIO_VAL_VALUE ,
NULL,
NULL,
NULL,
NULL,
0);
The MMIO gpio library takes care of shadowing the direction and all.
It also will implement get/set_multiple() for you for free.
So seriously consider making one gpio_chip per bank.
> +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> +static void aspeed_sgpio_irq_ack(struct irq_data *d)
> +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
> +static void aspeed_sgpio_irq_mask(struct irq_data *d)
> +static void aspeed_sgpio_irq_unmask(struct irq_data *d)
> +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
> +static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
> +{
> + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> + struct irq_chip *ic = irq_desc_get_chip(desc);
> + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> + unsigned int i, p, girq;
> + unsigned long reg;
> +
> + chained_irq_enter(ic, desc);
> +
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> +
> + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> +
> + for_each_set_bit(p, ®, 32) {
> + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> + generic_handle_irq(girq);
> + }
> +
> + }
This also gets really complex with one driver for all the banks.
> + /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
> + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
(...)
> +static int __init aspeed_sgpio_probe(struct platform_device *pdev)
> +{
> + struct aspeed_sgpio *gpio;
> + u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> + int rc;
> + unsigned long apb_freq;
> +
> + /* initialize allocated memory with zeros */
No need for this comment, developers know what "kzalloc" means.
> + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
> + if (rc < 0) {
> + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> + return -EINVAL;
> + }
> +
> + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(gpio->pclk)) {
> + dev_err(&pdev->dev, "devm_clk_get failed\n");
> + return PTR_ERR(gpio->pclk);
> + }
> +
> + apb_freq = clk_get_rate(gpio->pclk);
> +
> + /*
> + * From the datasheet,
> + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> + * period = 2 * (GPIO254[31:16] + 1) / PCLK
> + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> + * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> + * GPIO254[31:16] = PCLK / (frequency * 2) - 1
> + */
> + if (sgpio_freq == 0)
> + return -EINVAL;
> +
> + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> +
> + if (sgpio_clk_div > (1 << 16) - 1)
> + return -EINVAL;
> +
> + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> + ASPEED_SGPIO_ENABLE,
> + gpio->base + ASPEED_SGPIO_CTRL);
This is a separate clock driver.
Break this out as a separate clk device that the other GPIOs
grab.
Put this in drivers/clk/clk-aspeed-gpio.c or wherever appropriate
with some
reg = <0xnnnnnn54 4>;
Then let the GPIO driver grab this clock. This makes it possible
to use a per-gpio-bank split of the GPIO chips.
It looks a bit complicated but this will work so much better because
the clock code is in the clock subsystem and the GPIO is split up
and becomes a very small driver since it can use gpio MMIO.
Yours,
Linus Walleij
^ permalink raw reply
* gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-19 20:51 UTC (permalink / raw)
To: linux-aspeed
Hello Andrew,
Thanks for reviewing and suggestions.
please see my inline comments. I just submitted v5 with a proper series title,
please help review them.
In AST2500 datasheet, it says:
"Support up to 80 SGPIO input ports and 80 output ports concurrently only at
the cost of 4 control pins".
Would you please give us some hints on how to configure the SGPIO pins so that
we can use them as input and output 'concurrently' ?
Thanks,
--Hongwei
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, July 17, 2019 9:45 PM
> To: Hongwei Zhang; Bartosz Golaszewski; Joel Stanley; Linus Walleij
> Cc: linux-gpio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org;
> linux-kernel at vger.kernel.org
> Subject: Re: [PATCH 2/3 v4] ARM: dts: aspeed: Add SGPIO driver
>
> Hello Hongwei,
>
> The driver is shaping up! I have a few remaining nitpicks below:
>
> The first is the patch subject:
>
> [PATCH 2/3 v4] ARM: dts: aspeed: Add SGPIO driver
>
> I think one of the first iterations of the patch included the devicetree changes. It doesn't any more, as
> the devicetree updates should be done in a separate patch, however the subject is now inaccurate.
> Disregarding the [PATCH ...] prefix, it should be:
>
> gpio: aspeed: Add SPGIO driver
>
> As for the [PATCH ...] prefix business, both your bindings patch and your driver patch are listed as patch
> 2/3, and there's no third patch in the series. `git format-patch` takes care of all this for you - I encourage
> you to get `git send-email` working so you can use it to send the patches produced from `git format-
> patch` (as they won't have these problems). Further, if you're sending multiple patches it's useful to
> include a cover letter as the patches will be properly threaded underneath it, which helps keep related
> patches together and provides a place to talk about your patches that isn't the commit message.
>
the v5 is submitted with properly combined patches.
> On Thu, 18 Jul 2019, at 07:00, Hongwei Zhang wrote:
> > Add SGPIO driver support for Aspeed AST2500 SoC.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > drivers/gpio/sgpio-aspeed.c | 518
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 518 insertions(+)
> > create mode 100644 drivers/gpio/sgpio-aspeed.c
> >
> > diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
> > new file mode 100644 index 0000000..715052c
> > --- /dev/null
> > +++ b/drivers/gpio/sgpio-aspeed.c
> > @@ -0,0 +1,518 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2019 American Megatrends International LLC.
> > + *
> > + * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in> */
> > +
> > +#include <linux/gpio/driver.h>
> > +#include <linux/gpio/aspeed.h>
> > +#include <linux/hashtable.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/string.h>
> > +#include <linux/gpio.h>
>
> It would be nice if these were sorted alphabetically. Not overly fussed though.
>
done.
> > +
> > +#define MAX_NR_SGPIO 80
> > +
> > +#define ASPEED_SGPIO_CTRL 0x54
> > +
> > +#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
> > +#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
> > +#define ASPEED_SGPIO_ENABLE BIT(0)
> > +
> > +struct aspeed_sgpio {
> > + struct gpio_chip chip;
> > + struct clk *pclk;
> > + spinlock_t lock;
> > + void __iomem *base;
> > + uint32_t dir_in[3];
> > + int irq;
> > +};
> > +
> > +struct aspeed_sgpio_bank {
> > + uint16_t val_regs;
> > + uint16_t rdata_reg;
> > + uint16_t irq_regs;
> > + const char names[4][3];
> > +};
> > +
> > +/*
> > + * Note: The "value" register returns the input value when the GPIO is
> > + * configured as an input.
> > + *
> > + * The "rdata" register returns the output value when the GPIO is
> > + * configured as an output.
> > + */
> > +static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
> > + {
> > + .val_regs = 0x0000,
> > + .rdata_reg = 0x0070,
> > + .irq_regs = 0x0004,
> > + .names = { "A", "B", "C", "D" },
> > + },
> > + {
> > + .val_regs = 0x001C,
> > + .rdata_reg = 0x0074,
> > + .irq_regs = 0x0020,
> > + .names = { "E", "F", "G", "H" },
> > + },
> > + {
> > + .val_regs = 0x0038,
> > + .rdata_reg = 0x0078,
> > + .irq_regs = 0x003C,
> > + .names = { "I", "J" },
> > + },
> > +};
> > +
> > +enum aspeed_sgpio_reg {
> > + reg_val,
> > + reg_rdata,
> > + reg_irq_enable,
> > + reg_irq_type0,
> > + reg_irq_type1,
> > + reg_irq_type2,
> > + reg_irq_status,
> > +};
> > +
> > +#define GPIO_VAL_VALUE 0x00
> > +#define GPIO_IRQ_ENABLE 0x00
> > +#define GPIO_IRQ_TYPE0 0x04
> > +#define GPIO_IRQ_TYPE1 0x08
> > +#define GPIO_IRQ_TYPE2 0x0C
> > +#define GPIO_IRQ_STATUS 0x10
> > +
> > +/* This will be resolved at compile time */ static inline void
> > +__iomem *bank_reg(struct aspeed_sgpio *gpio,
> > + const struct aspeed_sgpio_bank *bank,
> > + const enum aspeed_sgpio_reg reg) {
> > + switch (reg) {
> > + case reg_val:
> > + return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
> > + case reg_rdata:
> > + return gpio->base + bank->rdata_reg;
> > + case reg_irq_enable:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
> > + case reg_irq_type0:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
> > + case reg_irq_type1:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
> > + case reg_irq_type2:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
> > + case reg_irq_status:
> > + return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
> > + default:
> > + /* acturally if code runs to here, it's an error case */
> > + BUG_ON(1);
> > + }
> > +}
> > +
> > +#define GPIO_BANK(x) ((x) >> 5)
> > +#define GPIO_OFFSET(x) ((x) & 0x1f)
> > +#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
> > +
> > +static const struct aspeed_sgpio_bank *to_bank(unsigned int offset) {
> > + unsigned int bank = GPIO_BANK(offset);
> > +
> > + WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
> > + return &aspeed_sgpio_banks[bank];
> > +}
> > +
> > +static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int
> > +offset) {
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > + unsigned long flags;
> > + enum aspeed_sgpio_reg reg;
> > + bool is_input;
> > + int rc = 0;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
> > + reg = is_input ? reg_val : reg_rdata;
> > + rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return rc;
> > +}
> > +
> > +static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + const struct aspeed_sgpio_bank *bank = to_bank(offset);
> > + unsigned long flags;
> > + void __iomem *addr;
> > + u32 reg = 0;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_val);
> > +
> > + if (val)
> > + reg |= GPIO_BIT(offset);
> > + else
> > + reg &= ~GPIO_BIT(offset);
> > +
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int
> > offset)
> > +{
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > + gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int
> > offset, int val)
> > +{
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > + gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return 0;
> > +}
> > +
> > +static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned
> > int offset)
> > +{
> > + int dir_status;
> > + struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > + dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + return dir_status;
> > +
> > +}
> > +
> > +static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
> > + struct aspeed_sgpio **gpio,
> > + const struct aspeed_sgpio_bank **bank,
> > + u32 *bit, int *offset)
> > +{
> > + struct aspeed_sgpio *internal;
> > +
> > + *offset = irqd_to_hwirq(d);
> > + internal = irq_data_get_irq_chip_data(d);
> > + WARN_ON(!internal);
> > +
> > + *gpio = internal;
> > + *bank = to_bank(*offset);
> > + *bit = GPIO_BIT(*offset);
> > +}
> > +
> > +static void aspeed_sgpio_irq_ack(struct irq_data *d) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *status_addr;
> > + int offset;
> > + u32 bit;
> > +
> > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > +
> > + status_addr = bank_reg(gpio, bank, reg_irq_status);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + iowrite32(bit, status_addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set) {
> > + const struct aspeed_sgpio_bank *bank;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + u32 reg, bit;
> > + void __iomem *addr;
> > + int offset;
> > +
> > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > + addr = bank_reg(gpio, bank, reg_irq_enable);
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + reg = ioread32(addr);
> > + if (set)
> > + reg |= bit;
> > + else
> > + reg &= ~bit;
> > +
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags); }
> > +
> > +static void aspeed_sgpio_irq_mask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, false); }
> > +
> > +static void aspeed_sgpio_irq_unmask(struct irq_data *d) {
> > + aspeed_sgpio_irq_set_mask(d, true);
> > +}
> > +
> > +static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int
> > +type) {
> > + u32 type0 = 0;
> > + u32 type1 = 0;
> > + u32 type2 = 0;
> > + u32 bit, reg;
> > + const struct aspeed_sgpio_bank *bank;
> > + irq_flow_handler_t handler;
> > + struct aspeed_sgpio *gpio;
> > + unsigned long flags;
> > + void __iomem *addr;
> > + int offset;
> > +
> > + irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
> > +
> > + switch (type & IRQ_TYPE_SENSE_MASK) {
> > + case IRQ_TYPE_EDGE_BOTH:
> > + type2 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_RISING:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_EDGE_FALLING:
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + type0 |= bit;
> > + /* fall through */
> > + case IRQ_TYPE_LEVEL_LOW:
> > + type1 |= bit;
> > + handler = handle_level_irq;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + spin_lock_irqsave(&gpio->lock, flags);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type0);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type0;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type1);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type1;
> > + iowrite32(reg, addr);
> > +
> > + addr = bank_reg(gpio, bank, reg_irq_type2);
> > + reg = ioread32(addr);
> > + reg = (reg & ~bit) | type2;
> > + iowrite32(reg, addr);
> > +
> > + spin_unlock_irqrestore(&gpio->lock, flags);
> > +
> > + irq_set_handler_locked(d, handler);
> > +
> > + return 0;
> > +}
> > +
> > +static void aspeed_sgpio_irq_handler(struct irq_desc *desc) {
> > + struct gpio_chip *gc = irq_desc_get_handler_data(desc);
> > + struct irq_chip *ic = irq_desc_get_chip(desc);
> > + struct aspeed_sgpio *data = gpiochip_get_data(gc);
> > + unsigned int i, p, girq;
> > + unsigned long reg;
> > +
> > + chained_irq_enter(ic, desc);
> > +
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
> > +
> > + reg = ioread32(bank_reg(data, bank, reg_irq_status));
> > +
> > + for_each_set_bit(p, ®, 32) {
> > + girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
> > + generic_handle_irq(girq);
> > + }
> > +
> > + }
> > +
> > + chained_irq_exit(ic, desc);
> > +}
> > +
> > +static struct irq_chip aspeed_sgpio_irqchip = {
> > + .name = "aspeed-sgpio",
> > + .irq_ack = aspeed_sgpio_irq_ack,
> > + .irq_mask = aspeed_sgpio_irq_mask,
> > + .irq_unmask = aspeed_sgpio_irq_unmask,
> > + .irq_set_type = aspeed_sgpio_set_type,
> > +};
> > +
> > +static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
> > + struct platform_device *pdev)
> > +{
> > + int rc, i;
> > + const struct aspeed_sgpio_bank *bank;
> > +
> > + rc = platform_get_irq(pdev, 0);
> > + if (rc < 0)
> > + return rc;
> > +
> > + gpio->irq = rc;
> > +
> > + /* Disable IRQ and clear Interrupt status registers for all SPGIO
> > Pins. */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* disable irq enable bits */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
> > + /* clear status bits */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
> > + }
> > +
> > + rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
> > + 0, handle_bad_irq, IRQ_TYPE_NONE);
> > + if (rc) {
> > + dev_info(&pdev->dev, "Could not add irqchip\n");
> > + return rc;
> > + }
> > +
> > + gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
> > + gpio->irq, aspeed_sgpio_irq_handler);
> > +
> > + /* set IRQ settings and Enable Interrupt */
> > + for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
> > + bank = &aspeed_sgpio_banks[i];
> > + /* set falling or level-low irq */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
> > + /* trigger type is edge */
> > + iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
> > + /* dual edge trigger mode. */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
> > + /* enable irq */
> > + iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id aspeed_sgpio_of_table[] = {
> > + { .compatible = "aspeed,ast2400-sgpio" },
> > + { .compatible = "aspeed,ast2500-sgpio" },
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
> > +
> > +static int __init aspeed_sgpio_probe(struct platform_device *pdev) {
> > + struct aspeed_sgpio *gpio;
> > + u32 nr_gpios, sgpio_freq, sgpio_clk_div;
> > + int rc;
> > + unsigned long apb_freq;
> > +
> > + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + gpio->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(gpio->base))
> > + return PTR_ERR(gpio->base);
> > +
> > + rc = of_property_read_u32(pdev->dev.of_node, "nr-gpios", &nr_gpios);
>
> I just checked and the standard property is "ngpios", not "nr-gpios"[1]. Please fix this and the bindings
> document.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindi
> ngs/gpio/gpio.txt?h=v5.2#n141
>
changed to ngpios
> > + if ((rc < 0) || (nr_gpios > MAX_NR_SGPIO)) {
> > + dev_err(&pdev->dev, "Could not read nr-gpios property\n");
>
> Triggering this error message with the `nr_gpios > MAX_NR_SGPIO` condition is a bit confusing. I think
> you should check that in a separate conditional block if you would like to issue a dev_err():
>
> if (rc < 0) {
> dev_err(&pdev->dev, "Could not read nr-gpios property\n");
> return -EINVAL;
> } else if (nr_gpios > MAX_NR_GPIOS) {
> dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
> MAX_NR_SGPIO, nr_gpios);
> return -EINVAL;
> }
>
done, it's nicer with your suggestion.
> > + return -EINVAL;
> > + }
> > +
> > + rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
> > &sgpio_freq);
> > + if (rc < 0) {
> > + dev_err(&pdev->dev, "Could not read bus-frequency property\n");
> > + return -EINVAL;
> > + }
> > +
> > + gpio->pclk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(gpio->pclk)) {
> > + dev_err(&pdev->dev, "devm_clk_get failed\n");
> > + return PTR_ERR(gpio->pclk);
> > + }
> > +
> > + apb_freq = clk_get_rate(gpio->pclk);
> > +
> > + /*
> > + * From the datasheet,
> > + * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
> > + * period = 2 * (GPIO254[31:16] + 1) / PCLK
> > + * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
> > + * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
> > + * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
> > + * GPIO254[31:16] = PCLK / (frequency * 2) - 1
> > + */
> > + if (sgpio_freq == 0)
> > + return -EINVAL;
> > +
> > + sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
> > +
> > + if (sgpio_clk_div > (1 << 16) - 1)
> > + return -EINVAL;
> > +
> > + iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
> > + FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
> > + ASPEED_SGPIO_ENABLE,
> > + gpio->base + ASPEED_SGPIO_CTRL);
> > +
> > + spin_lock_init(&gpio->lock);
> > +
> > + gpio->chip.parent = &pdev->dev;
> > + gpio->chip.ngpio = nr_gpios;
> > + gpio->chip.direction_input = aspeed_sgpio_dir_in;
> > + gpio->chip.direction_output = aspeed_sgpio_dir_out;
> > + gpio->chip.get_direction = aspeed_sgpio_get_direction;
> > + gpio->chip.request = NULL;
> > + gpio->chip.free = NULL;
> > + gpio->chip.get = aspeed_sgpio_get;
> > + gpio->chip.set = aspeed_sgpio_set;
> > + gpio->chip.set_config = NULL;
> > + gpio->chip.label = dev_name(&pdev->dev);
> > + gpio->chip.base = ARCH_NR_GPIOS - MAX_NR_SGPIO;
>
> Please set this to -1 so the base is chosen for you by the gpio subsystem.
>
done
> > +
> > + /* set all SGPIO pins as input. */
> > + memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
>
> You've used devm_kzalloc() above, so the memory is already zeroed. You can remove the memset(),
> maybe just shift the comment to the devm_kzalloc().
>
here we need to initialize dir_in as its default input state, so memset with 0xff.
I changed comment a little bit to be more specific. also added a comment
at above devm_kzalloc().
> Other than these small issues, looks good to me. Thanks for iterating on it.
>
> Andrew
>
> > +
> > + rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
> > + if (rc < 0)
> > + return rc;
> > +
> > + return aspeed_sgpio_setup_irqs(gpio, pdev); }
> > +
> > +static struct platform_driver aspeed_sgpio_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = aspeed_sgpio_of_table,
> > + },
> > +};
> > +
> > +module_platform_driver_probe(aspeed_sgpio_driver,
> > +aspeed_sgpio_probe); MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.7.4
> >
> >
^ permalink raw reply
* dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-19 20:37 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563394325-15941-1-git-send-email-hongweiz@ami.com>
Hello Andrew,
Thanks for reviewing and please see my inline comments.
--Hongwei
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Wednesday, July 17, 2019 9:48 PM
> To: Hongwei Zhang; Joel Stanley; Linus Walleij; devicetree at vger.kernel.org
> Cc: Rob Herring; Mark Rutland; Bartosz Golaszewski; linux-aspeed at lists.ozlabs.org; linux-
> kernel at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-gpio at vger.kernel.org
> Subject: Re: [PATCH 2/3 v4] dt-bindings: gpio: aspeed: Add SGPIO support
>
> The subject is largely correct, but please see the discussion on the driver patch about how to clean up
> the [PATCH ...] prefix.
>
> On Thu, 18 Jul 2019, at 05:42, Hongwei Zhang wrote:
> > Add bindings to support SGPIO on AST2400 or AST2500.
> >
> > Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
> > ---
> > .../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
> > 1 file changed, 55 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> >
> > diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > new file mode 100644
> > index 0000000..2d6305e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
> > @@ -0,0 +1,55 @@
> > +Aspeed SGPIO controller Device Tree Bindings
> > +-------------------------------------------
> > +
> > +This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80
> > full
> > +featured Serial GPIOs. Each of the Serial GPIO pins can be programmed
> > to
> > +support the following options:
> > +- Support interrupt option for each input port and various interrupt
> > + sensitivity option (level-high, level-low, edge-high, edge-low)
> > +- Support reset tolerance option for each output port
> > +- Directly connected to APB bus and its shift clock is from APB bus
> > clock
> > + divided by a programmable value.
> > +- Co-work with external signal-chained TTL components
> > +(74LV165/74LV595)
> > +
> > +
> > +Required properties:
> > +
> > +- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
> > +
> > +- #gpio-cells : Should be two
> > + - First cell is the GPIO line number
> > + - Second cell is used to specify optional
> > + parameters (unused)
> > +
> > +- reg : Address and length of the register set for the device
> > +- gpio-controller : Marks the device node as a GPIO controller
> > +- interrupts : Interrupt specifier (see interrupt bindings for
> > + details)
> > +
> > +- interrupt-controller : Mark the GPIO controller as an
> > interrupt-controller
> > +
> > +- nr-gpios : number of GPIO pins to serialise.
> > + (should be multiple of 8, up to 80 pins)
>
> Please change the property name to "ngpios", as per the generic GPIO bindings[1].
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindi
> ngs/gpio/gpio.txt?h=v5.2#n141
done
>
> Cheers,
>
> Andrew
>
> > +
> > +- clocks : A phandle to the APB clock for SGPM clock
> > division
> > +
> > +- bus-frequency : SGPM CLK frequency
> > +
> > +
> > +The sgpio and interrupt properties are further described in their
> > respective bindings documentation:
> > +
> > +- Documentation/devicetree/bindings/sgpio/gpio.txt
> > +-
> > +Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > +
> > + Example:
> > + sgpio: sgpio at 1e780200 {
> > + #gpio-cells = <2>;
> > + compatible = "aspeed,ast2500-sgpio";
> > + gpio-controller;
> > + interrupts = <40>;
> > + reg = <0x1e780200 0x0100>;
> > + clocks = <&syscon ASPEED_CLK_APB>;
> > + interrupt-controller;
> > + nr-gpios = <8>;
> > + bus-frequency = <12000000>;
> > + };
> > --
> > 2.7.4
> >
> >
^ permalink raw reply
* [PATCH linux dev-5.2] ARM: dts: aspeed: swift: Fix FSI GPIOs
From: Matt Spinler @ 2019-07-19 20:30 UTC (permalink / raw)
To: linux-aspeed
From: Matt Spinler <spinler@us.ibm.com>
Change the FSI clock and data GPIOs to match what the hardware turned
out to use.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
---
arch/arm/boot/dts/aspeed-bmc-opp-swift.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
index caac895c60b4..f14f745b34ca 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
@@ -207,8 +207,8 @@
#size-cells = <0>;
no-gpio-delays;
- clock-gpios = <&gpio ASPEED_GPIO(AA, 0) GPIO_ACTIVE_HIGH>;
- data-gpios = <&gpio ASPEED_GPIO(E, 0) GPIO_ACTIVE_HIGH>;
+ clock-gpios = <&gpio ASPEED_GPIO(P, 1) GPIO_ACTIVE_HIGH>;
+ data-gpios = <&gpio ASPEED_GPIO(P, 2) GPIO_ACTIVE_HIGH>;
mux-gpios = <&gpio ASPEED_GPIO(P, 4) GPIO_ACTIVE_HIGH>;
enable-gpios = <&gpio ASPEED_GPIO(P, 0) GPIO_ACTIVE_HIGH>;
trans-gpios = <&gpio ASPEED_GPIO(P, 3) GPIO_ACTIVE_HIGH>;
--
2.22.0
^ permalink raw reply related
* [PATCH linux dev-5.2] ARM: dts: aspeed: swift: Fix FSI GPIOs
From: Matt Spinler @ 2019-07-19 20:21 UTC (permalink / raw)
To: linux-aspeed
Change the FSI clock and data GPIOs to match what the hardware turned
out to use.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
---
arch/arm/boot/dts/aspeed-bmc-opp-swift.dts | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
index caac895c60b4..f14f745b34ca 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-swift.dts
@@ -207,8 +207,8 @@
#size-cells = <0>;
no-gpio-delays;
- clock-gpios = <&gpio ASPEED_GPIO(AA, 0) GPIO_ACTIVE_HIGH>;
- data-gpios = <&gpio ASPEED_GPIO(E, 0) GPIO_ACTIVE_HIGH>;
+ clock-gpios = <&gpio ASPEED_GPIO(P, 1) GPIO_ACTIVE_HIGH>;
+ data-gpios = <&gpio ASPEED_GPIO(P, 2) GPIO_ACTIVE_HIGH>;
mux-gpios = <&gpio ASPEED_GPIO(P, 4) GPIO_ACTIVE_HIGH>;
enable-gpios = <&gpio ASPEED_GPIO(P, 0) GPIO_ACTIVE_HIGH>;
trans-gpios = <&gpio ASPEED_GPIO(P, 3) GPIO_ACTIVE_HIGH>;
--
2.22.0
^ permalink raw reply related
* [v5 2/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-19 19:24 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563564291-9692-1-git-send-email-hongweiz@ami.com>
Add SGPIO driver support for Aspeed AST2500 SoC.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
drivers/gpio/sgpio-aspeed.c | 522 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 522 insertions(+)
create mode 100644 drivers/gpio/sgpio-aspeed.c
diff --git a/drivers/gpio/sgpio-aspeed.c b/drivers/gpio/sgpio-aspeed.c
new file mode 100644
index 0000000..c024c0a
--- /dev/null
+++ b/drivers/gpio/sgpio-aspeed.c
@@ -0,0 +1,522 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2019 American Megatrends International LLC.
+ *
+ * Author: Karthikeyan Mani <karthikeyanm@amiindia.co.in>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/gpio/driver.h>
+#include <linux/hashtable.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/string.h>
+
+#define MAX_NR_SGPIO 80
+
+#define ASPEED_SGPIO_CTRL 0x54
+
+#define ASPEED_SGPIO_PINS_MASK GENMASK(9, 6)
+#define ASPEED_SGPIO_CLK_DIV_MASK GENMASK(31, 16)
+#define ASPEED_SGPIO_ENABLE BIT(0)
+
+struct aspeed_sgpio {
+ struct gpio_chip chip;
+ struct clk *pclk;
+ spinlock_t lock;
+ void __iomem *base;
+ uint32_t dir_in[3];
+ int irq;
+};
+
+struct aspeed_sgpio_bank {
+ uint16_t val_regs;
+ uint16_t rdata_reg;
+ uint16_t irq_regs;
+ const char names[4][3];
+};
+
+/*
+ * Note: The "value" register returns the input value when the GPIO is
+ * configured as an input.
+ *
+ * The "rdata" register returns the output value when the GPIO is
+ * configured as an output.
+ */
+static const struct aspeed_sgpio_bank aspeed_sgpio_banks[] = {
+ {
+ .val_regs = 0x0000,
+ .rdata_reg = 0x0070,
+ .irq_regs = 0x0004,
+ .names = { "A", "B", "C", "D" },
+ },
+ {
+ .val_regs = 0x001C,
+ .rdata_reg = 0x0074,
+ .irq_regs = 0x0020,
+ .names = { "E", "F", "G", "H" },
+ },
+ {
+ .val_regs = 0x0038,
+ .rdata_reg = 0x0078,
+ .irq_regs = 0x003C,
+ .names = { "I", "J" },
+ },
+};
+
+enum aspeed_sgpio_reg {
+ reg_val,
+ reg_rdata,
+ reg_irq_enable,
+ reg_irq_type0,
+ reg_irq_type1,
+ reg_irq_type2,
+ reg_irq_status,
+};
+
+#define GPIO_VAL_VALUE 0x00
+#define GPIO_IRQ_ENABLE 0x00
+#define GPIO_IRQ_TYPE0 0x04
+#define GPIO_IRQ_TYPE1 0x08
+#define GPIO_IRQ_TYPE2 0x0C
+#define GPIO_IRQ_STATUS 0x10
+
+/* This will be resolved at compile time */
+static inline void __iomem *bank_reg(struct aspeed_sgpio *gpio,
+ const struct aspeed_sgpio_bank *bank,
+ const enum aspeed_sgpio_reg reg)
+{
+ switch (reg) {
+ case reg_val:
+ return gpio->base + bank->val_regs + GPIO_VAL_VALUE;
+ case reg_rdata:
+ return gpio->base + bank->rdata_reg;
+ case reg_irq_enable:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_ENABLE;
+ case reg_irq_type0:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE0;
+ case reg_irq_type1:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE1;
+ case reg_irq_type2:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_TYPE2;
+ case reg_irq_status:
+ return gpio->base + bank->irq_regs + GPIO_IRQ_STATUS;
+ default:
+ /* acturally if code runs to here, it's an error case */
+ BUG_ON(1);
+ }
+}
+
+#define GPIO_BANK(x) ((x) >> 5)
+#define GPIO_OFFSET(x) ((x) & 0x1f)
+#define GPIO_BIT(x) BIT(GPIO_OFFSET(x))
+
+static const struct aspeed_sgpio_bank *to_bank(unsigned int offset)
+{
+ unsigned int bank = GPIO_BANK(offset);
+
+ WARN_ON(bank >= ARRAY_SIZE(aspeed_sgpio_banks));
+ return &aspeed_sgpio_banks[bank];
+}
+
+static int aspeed_sgpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ enum aspeed_sgpio_reg reg;
+ bool is_input;
+ int rc = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ is_input = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ reg = is_input ? reg_val : reg_rdata;
+ rc = !!(ioread32(bank_reg(gpio, bank, reg)) & GPIO_BIT(offset));
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return rc;
+}
+
+static void aspeed_sgpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ const struct aspeed_sgpio_bank *bank = to_bank(offset);
+ unsigned long flags;
+ void __iomem *addr;
+ u32 reg = 0;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_val);
+
+ if (val)
+ reg |= GPIO_BIT(offset);
+ else
+ reg &= ~GPIO_BIT(offset);
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static int aspeed_sgpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] |= GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
+static int aspeed_sgpio_dir_out(struct gpio_chip *gc, unsigned int offset, int val)
+{
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ gpio->dir_in[GPIO_BANK(offset)] &= ~GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return 0;
+}
+
+static int aspeed_sgpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+ int dir_status;
+ struct aspeed_sgpio *gpio = gpiochip_get_data(gc);
+ unsigned long flags;
+
+ spin_lock_irqsave(&gpio->lock, flags);
+ dir_status = gpio->dir_in[GPIO_BANK(offset)] & GPIO_BIT(offset);
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ return dir_status;
+
+}
+
+static inline void irqd_to_aspeed_sgpio_data(struct irq_data *d,
+ struct aspeed_sgpio **gpio,
+ const struct aspeed_sgpio_bank **bank,
+ u32 *bit, int *offset)
+{
+ struct aspeed_sgpio *internal;
+
+ *offset = irqd_to_hwirq(d);
+ internal = irq_data_get_irq_chip_data(d);
+ WARN_ON(!internal);
+
+ *gpio = internal;
+ *bank = to_bank(*offset);
+ *bit = GPIO_BIT(*offset);
+}
+
+static void aspeed_sgpio_irq_ack(struct irq_data *d)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *status_addr;
+ int offset;
+ u32 bit;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ status_addr = bank_reg(gpio, bank, reg_irq_status);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ iowrite32(bit, status_addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_set_mask(struct irq_data *d, bool set)
+{
+ const struct aspeed_sgpio_bank *bank;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ u32 reg, bit;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+ addr = bank_reg(gpio, bank, reg_irq_enable);
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ reg = ioread32(addr);
+ if (set)
+ reg |= bit;
+ else
+ reg &= ~bit;
+
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+}
+
+static void aspeed_sgpio_irq_mask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, false);
+}
+
+static void aspeed_sgpio_irq_unmask(struct irq_data *d)
+{
+ aspeed_sgpio_irq_set_mask(d, true);
+}
+
+static int aspeed_sgpio_set_type(struct irq_data *d, unsigned int type)
+{
+ u32 type0 = 0;
+ u32 type1 = 0;
+ u32 type2 = 0;
+ u32 bit, reg;
+ const struct aspeed_sgpio_bank *bank;
+ irq_flow_handler_t handler;
+ struct aspeed_sgpio *gpio;
+ unsigned long flags;
+ void __iomem *addr;
+ int offset;
+
+ irqd_to_aspeed_sgpio_data(d, &gpio, &bank, &bit, &offset);
+
+ switch (type & IRQ_TYPE_SENSE_MASK) {
+ case IRQ_TYPE_EDGE_BOTH:
+ type2 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_RISING:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_EDGE_FALLING:
+ handler = handle_edge_irq;
+ break;
+ case IRQ_TYPE_LEVEL_HIGH:
+ type0 |= bit;
+ /* fall through */
+ case IRQ_TYPE_LEVEL_LOW:
+ type1 |= bit;
+ handler = handle_level_irq;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&gpio->lock, flags);
+
+ addr = bank_reg(gpio, bank, reg_irq_type0);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type0;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type1);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type1;
+ iowrite32(reg, addr);
+
+ addr = bank_reg(gpio, bank, reg_irq_type2);
+ reg = ioread32(addr);
+ reg = (reg & ~bit) | type2;
+ iowrite32(reg, addr);
+
+ spin_unlock_irqrestore(&gpio->lock, flags);
+
+ irq_set_handler_locked(d, handler);
+
+ return 0;
+}
+
+static void aspeed_sgpio_irq_handler(struct irq_desc *desc)
+{
+ struct gpio_chip *gc = irq_desc_get_handler_data(desc);
+ struct irq_chip *ic = irq_desc_get_chip(desc);
+ struct aspeed_sgpio *data = gpiochip_get_data(gc);
+ unsigned int i, p, girq;
+ unsigned long reg;
+
+ chained_irq_enter(ic, desc);
+
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ const struct aspeed_sgpio_bank *bank = &aspeed_sgpio_banks[i];
+
+ reg = ioread32(bank_reg(data, bank, reg_irq_status));
+
+ for_each_set_bit(p, ®, 32) {
+ girq = irq_find_mapping(gc->irq.domain, i * 32 + p);
+ generic_handle_irq(girq);
+ }
+
+ }
+
+ chained_irq_exit(ic, desc);
+}
+
+static struct irq_chip aspeed_sgpio_irqchip = {
+ .name = "aspeed-sgpio",
+ .irq_ack = aspeed_sgpio_irq_ack,
+ .irq_mask = aspeed_sgpio_irq_mask,
+ .irq_unmask = aspeed_sgpio_irq_unmask,
+ .irq_set_type = aspeed_sgpio_set_type,
+};
+
+static int aspeed_sgpio_setup_irqs(struct aspeed_sgpio *gpio,
+ struct platform_device *pdev)
+{
+ int rc, i;
+ const struct aspeed_sgpio_bank *bank;
+
+ rc = platform_get_irq(pdev, 0);
+ if (rc < 0)
+ return rc;
+
+ gpio->irq = rc;
+
+ /* Disable IRQ and clear Interrupt status registers for all SPGIO Pins. */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* disable irq enable bits */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_enable));
+ /* clear status bits */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_status));
+ }
+
+ rc = gpiochip_irqchip_add(&gpio->chip, &aspeed_sgpio_irqchip,
+ 0, handle_bad_irq, IRQ_TYPE_NONE);
+ if (rc) {
+ dev_info(&pdev->dev, "Could not add irqchip\n");
+ return rc;
+ }
+
+ gpiochip_set_chained_irqchip(&gpio->chip, &aspeed_sgpio_irqchip,
+ gpio->irq, aspeed_sgpio_irq_handler);
+
+ /* set IRQ settings and Enable Interrupt */
+ for (i = 0; i < ARRAY_SIZE(aspeed_sgpio_banks); i++) {
+ bank = &aspeed_sgpio_banks[i];
+ /* set falling or level-low irq */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type0));
+ /* trigger type is edge */
+ iowrite32(0x00000000, bank_reg(gpio, bank, reg_irq_type1));
+ /* dual edge trigger mode. */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_type2));
+ /* enable irq */
+ iowrite32(0xffffffff, bank_reg(gpio, bank, reg_irq_enable));
+ }
+
+ return 0;
+}
+
+static const struct of_device_id aspeed_sgpio_of_table[] = {
+ { .compatible = "aspeed,ast2400-sgpio" },
+ { .compatible = "aspeed,ast2500-sgpio" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, aspeed_sgpio_of_table);
+
+static int __init aspeed_sgpio_probe(struct platform_device *pdev)
+{
+ struct aspeed_sgpio *gpio;
+ u32 nr_gpios, sgpio_freq, sgpio_clk_div;
+ int rc;
+ unsigned long apb_freq;
+
+ /* initialize allocated memory with zeros */
+ gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ gpio->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(gpio->base))
+ return PTR_ERR(gpio->base);
+
+ rc = of_property_read_u32(pdev->dev.of_node, "ngpios", &nr_gpios);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read ngpios property\n");
+ return -EINVAL;
+ } else if (nr_gpios > MAX_NR_SGPIO) {
+ dev_err(&pdev->dev, "Number of GPIOs exceeds the maximum of %d: %d\n",
+ MAX_NR_SGPIO, nr_gpios);
+ return -EINVAL;
+ }
+
+ rc = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &sgpio_freq);
+ if (rc < 0) {
+ dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+ return -EINVAL;
+ }
+
+ gpio->pclk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(gpio->pclk)) {
+ dev_err(&pdev->dev, "devm_clk_get failed\n");
+ return PTR_ERR(gpio->pclk);
+ }
+
+ apb_freq = clk_get_rate(gpio->pclk);
+
+ /*
+ * From the datasheet,
+ * SGPIO period = 1/PCLK * 2 * (GPIO254[31:16] + 1)
+ * period = 2 * (GPIO254[31:16] + 1) / PCLK
+ * frequency = 1 / (2 * (GPIO254[31:16] + 1) / PCLK)
+ * frequency = PCLK / (2 * (GPIO254[31:16] + 1))
+ * frequency * 2 * (GPIO254[31:16] + 1) = PCLK
+ * GPIO254[31:16] = PCLK / (frequency * 2) - 1
+ */
+ if (sgpio_freq == 0)
+ return -EINVAL;
+
+ sgpio_clk_div = (apb_freq / (sgpio_freq * 2)) - 1;
+
+ if (sgpio_clk_div > (1 << 16) - 1)
+ return -EINVAL;
+
+ iowrite32(FIELD_PREP(ASPEED_SGPIO_CLK_DIV_MASK, sgpio_clk_div) |
+ FIELD_PREP(ASPEED_SGPIO_PINS_MASK, (nr_gpios / 8)) |
+ ASPEED_SGPIO_ENABLE,
+ gpio->base + ASPEED_SGPIO_CTRL);
+
+ spin_lock_init(&gpio->lock);
+
+ gpio->chip.parent = &pdev->dev;
+ gpio->chip.ngpio = nr_gpios;
+ gpio->chip.direction_input = aspeed_sgpio_dir_in;
+ gpio->chip.direction_output = aspeed_sgpio_dir_out;
+ gpio->chip.get_direction = aspeed_sgpio_get_direction;
+ gpio->chip.request = NULL;
+ gpio->chip.free = NULL;
+ gpio->chip.get = aspeed_sgpio_get;
+ gpio->chip.set = aspeed_sgpio_set;
+ gpio->chip.set_config = NULL;
+ gpio->chip.label = dev_name(&pdev->dev);
+ gpio->chip.base = -1;
+
+ /* set all SGPIO pins as input (1). */
+ memset(gpio->dir_in, 0xff, sizeof(gpio->dir_in));
+
+ rc = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
+ if (rc < 0)
+ return rc;
+
+ return aspeed_sgpio_setup_irqs(gpio, pdev);
+}
+
+static struct platform_driver aspeed_sgpio_driver = {
+ .driver = {
+ .name = KBUILD_MODNAME,
+ .of_match_table = aspeed_sgpio_of_table,
+ },
+};
+
+module_platform_driver_probe(aspeed_sgpio_driver, aspeed_sgpio_probe);
+MODULE_DESCRIPTION("Aspeed Serial GPIO Driver");
+MODULE_LICENSE("GPL");
--
2.7.4
^ permalink raw reply related
* [v5 1/2] dt-bindings: gpio: aspeed: Add SGPIO support
From: Hongwei Zhang @ 2019-07-19 19:24 UTC (permalink / raw)
To: linux-aspeed
In-Reply-To: <1563564291-9692-1-git-send-email-hongweiz@ami.com>
Add bindings to support SGPIO on AST2400 or AST2500.
Signed-off-by: Hongwei Zhang <hongweiz@ami.com>
---
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 ++++++++++++++++++++++
1 file changed, 55 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
diff --git a/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
new file mode 100644
index 0000000..f9ed438
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
@@ -0,0 +1,55 @@
+Aspeed SGPIO controller Device Tree Bindings
+-------------------------------------------
+
+This SGPIO controller is for ASPEED AST2500 SoC, it supports up to 80 full
+featured Serial GPIOs. Each of the Serial GPIO pins can be programmed to
+support the following options:
+- Support interrupt option for each input port and various interrupt
+ sensitivity option (level-high, level-low, edge-high, edge-low)
+- Support reset tolerance option for each output port
+- Directly connected to APB bus and its shift clock is from APB bus clock
+ divided by a programmable value.
+- Co-work with external signal-chained TTL components (74LV165/74LV595)
+
+
+Required properties:
+
+- compatible : Either "aspeed,ast2400-sgpio" or "aspeed,ast2500-sgpio"
+
+- #gpio-cells : Should be two
+ - First cell is the GPIO line number
+ - Second cell is used to specify optional
+ parameters (unused)
+
+- reg : Address and length of the register set for the device
+- gpio-controller : Marks the device node as a GPIO controller
+- interrupts : Interrupt specifier (see interrupt bindings for
+ details)
+
+- interrupt-controller : Mark the GPIO controller as an interrupt-controller
+
+- ngpios : number of GPIO pins to serialise.
+ (should be multiple of 8, up to 80 pins)
+
+- clocks : A phandle to the APB clock for SGPM clock division
+
+- bus-frequency : SGPM CLK frequency
+
+
+The sgpio and interrupt properties are further described in their respective bindings documentation:
+
+- Documentation/devicetree/bindings/sgpio/gpio.txt
+- Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+
+ Example:
+ sgpio: sgpio at 1e780200 {
+ #gpio-cells = <2>;
+ compatible = "aspeed,ast2500-sgpio";
+ gpio-controller;
+ interrupts = <40>;
+ reg = <0x1e780200 0x0100>;
+ clocks = <&syscon ASPEED_CLK_APB>;
+ interrupt-controller;
+ ngpios = <8>;
+ bus-frequency = <12000000>;
+ };
--
2.7.4
^ permalink raw reply related
* [v5 0/2] gpio: aspeed: Add SGPIO driver
From: Hongwei Zhang @ 2019-07-19 19:24 UTC (permalink / raw)
To: linux-aspeed
Hello,
This short series introduce dt-binding document and a driver for the
Aspeed AST2500 SGPIO controller. Please review.
The related SGPM pinmux dt-binding document, dts, and pinctrl driver
updates have been accepted and merged:
_http://patchwork.ozlabs.org/patch/1110210/
Hongwei Zhang (2):
dt-bindings: gpio: aspeed: Add SGPIO support
gpio: aspeed: Add SGPIO driver
.../devicetree/bindings/gpio/sgpio-aspeed.txt | 55 +++
drivers/gpio/sgpio-aspeed.c | 522 +++++++++++++++++++++
2 files changed, 577 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/sgpio-aspeed.txt
create mode 100644 drivers/gpio/sgpio-aspeed.c
--
2.7.4
^ 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