Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   <Kumaravel.Thiagarajan@microchip.com>
To:     <ilpo.jarvinen@linux.intel.com>
CC:     <gregkh@linuxfoundation.org>, <jirislaby@kernel.org>,
        <andy.shevchenko@gmail.com>, <u.kleine-koenig@pengutronix.de>,
        <johan@kernel.org>, <wander@redhat.com>,
        <etremblay@distech-controls.com>, <macro@orcam.me.uk>,
        <geert+renesas@glider.be>, <jk@ozlabs.org>,
        <phil.edworthy@renesas.com>, <lukas@wunner.de>,
        <linux-kernel@vger.kernel.org>, <linux-serial@vger.kernel.org>,
        <UNGLinuxDriver@microchip.com>
Subject: RE: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
 management functions to pci1xxxx's  quad-uart driver.
Thread-Topic: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
 management functions to pci1xxxx's  quad-uart driver.
Thread-Index: AQHYvJqN/ryPjdVWCUu2JC5qz9dINq3IxeGAgAKi/EA=
Date:   Fri, 2 Sep 2022 02:20:18 +0000
Message-ID: <BN8PR11MB3668D24337821FDE00FE7BB2E97A9@BN8PR11MB3668.namprd11.prod.outlook.com>
References: <20220830180054.1998296-1-kumaravel.thiagarajan@microchip.com>
 <20220830180054.1998296-3-kumaravel.thiagarajan@microchip.com>
 <8a47735-82d-809c-9b8c-a8d9d9a8d5c5@linux.intel.com>
In-Reply-To: <8a47735-82d-809c-9b8c-a8d9d9a8d5c5@linux.intel.com>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?lI6g/orZorxUzvoAp5Ds+hxmUhH9N3azcKC1zn9p3hI/9OoZw5KTfysMO3?=
 =?iso-8859-1?Q?6HNXY1r2zd3DWVP0vEtKJORvRzf6+c6jwdhgGYjO2TqybHlsSKYkJeocWC?=
 =?iso-8859-1?Q?1WeWvvE/XMC3Jm7fMTiivVleco3IdJFzxA8+CJQf65YcSms9VZBcIcauar?=
 =?iso-8859-1?Q?l9NMiHA1WQsmErkSetA7za7AzkOrbwW1V9UADdYeBWnAkxdwIWU2pp1nxL?=
 =?iso-8859-1?Q?SRhoGD5+chAgfKCyf6vpdhYhpU1zIx5k7Nr7rJ07vyV/f4J//pP+Hl7AtG?=
 =?iso-8859-1?Q?By24SgvqFnYDo7txTf+H3EH9g1zgoM7Nw3cIj22BKVlbjNm5bDX6qC3amu?=
 =?iso-8859-1?Q?a+glbXMbeZtMv/HUkBLteYERuGHQltenweg+Z6th6371RGi2llA1LRZW2W?=
 =?iso-8859-1?Q?Lq/+40Y9uhq0tF2IeA05lwMJNJqqkzl0tSENcmungeOx6Ll7zbaLdiqo48?=
 =?iso-8859-1?Q?gcTbhUO70e0FkAjDrwuVAwpvd/2vlcnryn/dN+Yb2PobUG19XlQ4uE3w5L?=
 =?iso-8859-1?Q?4FLPDds8JDSaJ1o8HU2tBi0ogryEK9bRAfVayD7yc0irqD2lZpgFpQ72NL?=
 =?iso-8859-1?Q?KIQxFNWvnQsPI8CRGXOv+QhKQdjqyq3HFvE1kGZksbr4SVwuAU6qDg6dVT?=
 =?iso-8859-1?Q?KCW8lcJ1xc1nwb0TRgDodODtjXGw4L5tjhbX8iYOd6NKDsw+l+TsV6z8z7?=
 =?iso-8859-1?Q?XgR3C1hKmIh1JUDohVIsZvqxiiQyW+ybye6Hk+Tkmce0cljY55EVTVHUmP?=
 =?iso-8859-1?Q?e/J7rVpy6LyE8cO8jqE1jX8Zvy/BPwZfiAxI+WP0QFZMSofzEkA4X8EUa7?=
 =?iso-8859-1?Q?T86PiCMLjXpRjBuQhbx7UyOdre4P2OeNBbebJUaJ+y/pbHzgdtK67bYQHZ?=
 =?iso-8859-1?Q?OJwrk+x5AYGT82BJKVs3lz/8SouFz2K3QeBDZYfTXSNI2tawOU/SLPmwmM?=
 =?iso-8859-1?Q?aMA5/5cfwQUGnrqGtB64lV8QRl7Y8uNj3mRijsZwJpDHX6HCYMpWWzydPw?=
 =?iso-8859-1?Q?S5jvZW1L4kNBj8GFZso/FgdXGT7P7bCTMJ/nLZMnt0TFGFH4nUUCvWpbEw?=
 =?iso-8859-1?Q?3PFx89gADrH7HSPMniQ6uNIkaEgkeqPhScTprqYcRJ9yz9qgWABCrUBE8G?=
 =?iso-8859-1?Q?7kBYW6e9vojVPWW3IzTsjN1gMK9/ZBtdQROAdtlfQZiQokBzVkush+Lfwi?=
 =?iso-8859-1?Q?QCzoQV7DFazlytPlNqdBkQBsrxTLWGcWeqsdICQd8fNP7AYEo91HobmdKC?=
 =?iso-8859-1?Q?XorrxOqIO4m6v+sA0x6Dqv62YXJ3gqllRPjIAC1UxWjD9tX+gdErBHfo3o?=
 =?iso-8859-1?Q?MZtUSZFBfbu95DfCRd1OV8M2HvkPRQSDrU6pL5opiWwQf76AGK8oPDRFo4?=
 =?iso-8859-1?Q?l/fwPNlWrbyYuqwnYcEO+9Ftk6u9p46Z56qW8lHxfIrmANxsqN7LTz752F?=
 =?iso-8859-1?Q?Hd1XS6FmmBkcKyibyTYDiixxzFZneNGhJCbrEJ1ofFgA2J36NOzOlMJclN?=
 =?iso-8859-1?Q?8cy7n2o5WxXoLI5rqqW7/eMGYuyOTmfEDalwuBRWWaPtcGMLizA8Gxt2b+?=
 =?iso-8859-1?Q?QK6lIutJGiui8VVii3Es+yrUKo82n31WrLyiQU8ZgvRRzn4jD1X6I0kDqy?=
 =?iso-8859-1?Q?Aou13bcAuXlj3wY3r+PpNoy5yt9+k9xguvPij4OzGySEqHGtMjv3/q1NE9?=
 =?iso-8859-1?Q?bkcRsrwqqa+JgzPa29I=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN8PR11MB3668.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5eaff1a9-e661-4db6-227c-08da8c89ae76
X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Sep 2022 02:20:19.0013
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: taELrWQi2BEyUDCKhDQ+NRKcjf0HvQ821KOx4L1z+4uNVjdMA/0ni1kWC9wBFetOJkD7x5eFc7TRevzKwz5hJFEcemxmjLscjndbEfw+HVJZlkG1eci8XGoT3gR6w+Hq
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1403
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Ilpo J=E4rvinen <ilpo.jarvinen@linux.intel.com>
> Sent: Wednesday, August 31, 2022 3:24 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@microchip.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; andy.shevchenko@gmail.com; u.kleine-
> koenig@pengutronix.de; johan@kernel.org; wander@redhat.com;
> etremblay@distech-controls.com; macro@orcam.me.uk;
> geert+renesas@glider.be; jk@ozlabs.org; phil.edworthy@renesas.com;
> Lukas Wunner <lukas@wunner.de>; LKML <linux-kernel@vger.kernel.org>;
> linux-serial <linux-serial@vger.kernel.org>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>
> Subject: Re: [PATCH v1 tty-next 2/2] 8250: microchip: pci1xxxx: Add power
> management functions to pci1xxxx's quad-uart driver.
>=20
>=20
> On Tue, 30 Aug 2022, Kumaravel Thiagarajan wrote:
>=20
> > pci1xxxx's quad-uart function has the capability to wake up the host
> > from suspend state. Enable wakeup before entering into suspend and
> > disable wakeup upon resume.
> >
> > Signed-off-by: Kumaravel Thiagarajan
> > <kumaravel.thiagarajan@microchip.com>
> > ---
> >  drivers/tty/serial/8250/8250_pci1xxxx.c | 122
> > ++++++++++++++++++++++++
> >  1 file changed, 122 insertions(+)
> >
> > diff --git a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > index 56852ae0585e..38c2a6a9e5d5 100644
> > --- a/drivers/tty/serial/8250/8250_pci1xxxx.c
> > +++ b/drivers/tty/serial/8250/8250_pci1xxxx.c
> > @@ -70,6 +70,7 @@
> >
> >  #define UART_PCI_CTRL_REG            0x80
> >  #define UART_PCI_CTRL_SET_MULTIPLE_MSI       BIT(4)
> > +#define UART_PCI_CTRL_D3_CLK_ENABLE  BIT(0)
>=20
> Reorder.
I have ordered this like - Register offset followed by individual bits from=
 MSB to LSB.
Should I reorder this based on some other criteria?

>=20
> > +static char pci1xxxx_port_suspend(int line)
>=20
> Why char???
I will modify this to bool.

>=20
> > +{
> > +     struct uart_8250_port *up =3D serial8250_get_port(line);
> > +     struct uart_port *port =3D &up->port;
> > +     unsigned long flags;
> > +     u8 wakeup_mask;
> > +     char ret =3D 0;
> > +
> > +     if (port->suspended =3D=3D 0 && port->dev) {
> > +             wakeup_mask =3D readb(up->port.membase +
> > + UART_WAKE_MASK_REG);
> > +
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl &=3D ~TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +
> > +             if ((wakeup_mask & UART_WAKE_SRCS) !=3D UART_WAKE_SRCS)
> > +                     ret =3D 0x01;
> > +             else
> > +                     ret =3D 0x00;
>=20
> Is it a bool or should you name these it with a #define?
bool is the correct data type. I will fix this.

>=20
> > +     }
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     return ret;
> > +}
> > +
> > +void pci1xxxx_port_resume(int line)
> > +{
> > +     struct uart_8250_port *up =3D serial8250_get_port(line);
> > +     struct uart_port *port =3D &up->port;
> > +     unsigned long flags;
> > +
> > +     writeb(UART_WAKE_SRCS, port->membase + UART_WAKE_REG);
> > +
> > +     if (port->suspended =3D=3D 0) {
> > +             spin_lock_irqsave(&port->lock, flags);
> > +             port->mctrl |=3D TIOCM_OUT2;
> > +             port->ops->set_mctrl(port, port->mctrl);
> > +             spin_unlock_irqrestore(&port->lock, flags);
> > +     }
> > +}
> > +
> > +static int pci1xxxx_suspend(struct device *dev) {
> > +     struct pci1xxxx_8250 *priv =3D dev_get_drvdata(dev);
> > +     struct pci_dev *pcidev =3D to_pci_dev(dev);
> > +     unsigned int data;
> > +     void __iomem *p;
> > +     char wakeup =3D 0;
> > +     int i;
> > +
> > +     for (i =3D 0; i < priv->nr; i++) {
> > +             if (priv->line[i] >=3D 0) {
> > +                     serial8250_suspend_port(priv->line[i]);
> > +                     wakeup |=3D pci1xxxx_port_suspend(priv->line[i]);
> > +             }
> > +     }
> > +
> > +     p =3D pci_ioremap_bar(pcidev, 0);
> > +     if (!p) {
> > +             dev_err(dev, "remapping of bar 0 memory failed");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     data =3D readl(p + UART_RESET_REG);
> > +     writel(data | UART_RESET_D3_RESET_DISABLE, p + UART_RESET_REG);
> > +
> > +     if (wakeup)
>=20
> It looks you'd want bool instead of char here.
Yes. I will modify this to bool.

Thank You.

Regards,
Kumaravel