Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1641979444-11661-1-git-send-email-hammerh0314@gmail.com>
 <1641979444-11661-3-git-send-email-hammerh0314@gmail.com> <fcd43c65-6201-9e44-061c-f04e39cef726@kernel.org>
 <CAOX-t54oA9V94d3901w2xKSagSzmXc9r=TDTtbgaSLfL1DxNbw@mail.gmail.com>
 <d6d3aa07-7bf1-2b6d-356f-ae13c7b9d6cd@kernel.org> <CAOX-t57KZb0hNDuhPsabkmkf_qOOLqyH3yuvkHP6UNwhLodWDg@mail.gmail.com>
 <2cde3ff0-5180-7c1e-82fd-7b58e41d462a@kernel.org> <CAOX-t573QkixRC7xa1KUOYXfL12Q+Ltxph9rX7V8tm2BMoqxgA@mail.gmail.com>
 <YfFQ7v4dXPMV7ypw@kroah.com>
In-Reply-To: <YfFQ7v4dXPMV7ypw@kroah.com>
From:   hammer hsieh <hammerh0314@gmail.com>
Date:   Fri, 28 Jan 2022 11:36:10 +0800
Message-ID: <CAOX-t54bRS0_kgg2DVoF3p8fx9VJh7xbyoTFcnAHnimLv40WbQ@mail.gmail.com>
Subject: Re: [PATCH v6 2/2] serial:sunplus-uart:Add Sunplus SoC UART Driver
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Jiri Slaby <jirislaby@kernel.org>, robh+dt@kernel.org,
        linux-serial@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, p.zabel@pengutronix.de,
        wells.lu@sunplus.com, "hammer.hsieh" <hammer.hsieh@sunplus.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi, Greg KH:

I review all driver again.
I think only startup and shutdown not good.
I will modify like below.
If you are ok, I will submit next patch.

static int sunplus_startup(struct uart_port *port)
{
        unsigned long flags;
        unsigned int isc;
        int ret;

        ret =3D request_irq(port->irq, sunplus_uart_irq, 0, "sunplus_uart",=
 port);
        if (ret)
                return ret;

        spin_lock_irqsave(&port->lock, flags);

        isc =3D readl(port->membase + SUP_UART_ISC); //add this line
        isc |=3D SUP_UART_ISC_RXM;
        writel(isc, port->membase + SUP_UART_ISC);

        spin_unlock_irqrestore(&port->lock, flags);

        return 0;
}

static void sunplus_shutdown(struct uart_port *port)
{
        unsigned long flags;
        unsigned int isc;

        spin_lock_irqsave(&port->lock, flags);

        isc =3D readl(port->membase + SUP_UART_ISC); //add this line
        isc &=3D ~(SUP_UART_ISC_RXM | SUP_UART_ISC_TXM); //add this line
        writel(isc, port->membase + SUP_UART_ISC); //modify this line

        spin_unlock_irqrestore(&port->lock, flags);

        free_irq(port->irq, port);
}

Greg KH <gregkh@linuxfoundation.org> =E6=96=BC 2022=E5=B9=B41=E6=9C=8826=E6=
=97=A5 =E9=80=B1=E4=B8=89 =E4=B8=8B=E5=8D=889:47=E5=AF=AB=E9=81=93=EF=BC=9A
>
> On Fri, Jan 14, 2022 at 10:22:56AM +0800, hammer hsieh wrote:
> > Jiri Slaby <jirislaby@kernel.org> =E6=96=BC 2022=E5=B9=B41=E6=9C=8813=
=E6=97=A5 =E9=80=B1=E5=9B=9B =E4=B8=8B=E5=8D=887:12=E5=AF=AB=E9=81=93=EF=BC=
=9A
> > >
> > > On 13. 01. 22, 11:56, hammer hsieh wrote:
> > > >> Could you explain me what posted write is and how does it not matt=
er in
> > > >> this case?
> > > >>
> > > >
> > > > Each UART ISC register contains
> > >
> > > No, you still don't follow what I write. Use your favorite web search
> > > for "posted write" and/or consult with your HW team.
> > >
> >
> > Maybe this time, we are on the same page.
> > Our SP7021 chipset is designed on ARM Cortex-A7 Quad core.
> > Register Access through AMBA(AXI bus), and it is non-cached.
> >
> > Did you mean
> > case1 have concern about "posted write", and you want to know why it no=
t matter?
> > case2 will be safer?
> >
> > Case1 :
> > spin_lock_irq_save()
> > writel(0, target register)
> > spin_unlock_irqrestore()
>
> A lock does not mean that your write made it to the device.  Please talk
> to the hardware designers to properly determine how to correctly write
> to the hardware and "know" that the write succeeded or not.  This driver
> does not seem to take that into consideration at all.
>
> thanks,
>
> greg k-h