Hi Noam,
On Sun, Oct 18, 2015 at 12:01:48PM +0300, Noam Camus wrote:
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index a0cdbf3..880f712 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -63,6 +63,9 @@ struct dw8250_data {
> struct clk *pclk;
> struct reset_control *rst;
> struct uart_8250_dma dma;
> + unsigned int (*serial_in)(void __iomem *addr);
> + void (*serial_out)(unsigned int value,
> + void __iomem *addr);
This is really ideal ..
> unsigned int skip_autocfg:1;
> unsigned int uart_16550_compatible:1;
> @@ -159,9 +162,9 @@ static void dw8250_serial_outq(struct uart_port *p, int offset, int value)
> }
> #endif /* CONFIG_64BIT */
>
> -static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> +static void dw8250_check_LCR(struct uart_port *p, int offset, int value)
> {
> - writel(value, p->membase + (offset << p->regshift));
> + struct dw8250_data *d = p->private_data;
>
> /* Make sure LCR write wasn't ignored */
> if (offset == UART_LCR) {
> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> return;
> dw8250_force_idle(p);
> - writel(value, p->membase + (UART_LCR << p->regshift));
> + d->serial_out(value,
> + p->membase + (UART_LCR << p->regshift));
> }
.. The way I see it, this is the only place where you would really
need the new private serial_in/out hooks, so why not just check the
iotype here and based on that use writeb/writel/iowrite32be or what
ever ..
<snip>
> static void dw8250_setup_port(struct uart_port *p)
> {
> struct uart_8250_port *up = up_to_u8250p(p);
> + struct dw8250_data *d = p->private_data;
> u32 reg;
>
> /*
> * If the Component Version Register returns zero, we know that
> * ADDITIONAL_FEATURES are not enabled. No need to go any further.
> */
> - reg = readl(p->membase + DW_UART_UCV);
> + reg = d->serial_in(p->membase + DW_UART_UCV);
> if (!reg)
> return;
>
> dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> (reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>
> - reg = readl(p->membase + DW_UART_CPR);
> + reg = d->serial_in(p->membase + DW_UART_CPR);
> if (!reg)
> return;
.. Here you could as well replace the direct readl calls with
serial_port_in calls.
> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device *pdev)
>
> err = device_property_read_u32(p->dev, "reg-io-width", &val);
> if (!err && val == 4) {
> - p->iotype = UPIO_MEM32;
> - p->serial_in = dw8250_serial_in32;
> - p->serial_out = dw8250_serial_out32;
> + p->iotype = of_device_is_big_endian(p->dev->of_node) ?
> + UPIO_MEM32BE : UPIO_MEM32;
If this has to be tied to DT, do this check in dw8250_quirks.
> + if (p->iotype == UPIO_MEM32) {
> + p->serial_in = dw8250_serial_in32;
> + p->serial_out = dw8250_serial_out32;
> + data->serial_in = _dw8250_serial_in32;
> + data->serial_out = _dw8250_serial_out32;
> + } else {
> + p->serial_in = dw8250_serial_in32be;
> + p->serial_out = dw8250_serial_out32be;
> + data->serial_in = _dw8250_serial_in32be;
> + data->serial_out = _dw8250_serial_out32be;
> + }
> }
>
> if (device_property_read_bool(p->dev, "dcd-override")) {
> @@ -466,6 +520,9 @@ static int dw8250_probe(struct platform_device *pdev)
>
> dw8250_quirks(p, data);
>
> + data->serial_in = _dw8250_serial_in32;
> + data->serial_out = _dw8250_serial_out32;
I don't think I understand what is going on here? You would never
actually even use _dw8250_serial_in32be/out32be, no?
Maybe I'm misunderstanding something.
Thanks,
--
heikki
On Wed, Dec 09, 2015 at 03:19:39PM +0200, Heikki Krogerus wrote:
> Hi Noam,
>
> On Sun, Oct 18, 2015 at 12:01:48PM +0300, Noam Camus wrote:
> > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> > index a0cdbf3..880f712 100644
> > --- a/drivers/tty/serial/8250/8250_dw.c
> > +++ b/drivers/tty/serial/8250/8250_dw.c
> > @@ -63,6 +63,9 @@ struct dw8250_data {
> > struct clk *pclk;
> > struct reset_control *rst;
> > struct uart_8250_dma dma;
> > + unsigned int (*serial_in)(void __iomem *addr);
> > + void (*serial_out)(unsigned int value,
> > + void __iomem *addr);
>
> This is really ideal ..
Meant to say _not_ ideal. Sorry about that.
Cheers,
--
heikki
>From: Heikki Krogerus [mailto:[email protected]]
>Sent: Wednesday, December 09, 2015 3:20 PM
>> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
>> if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
>> return;
>> dw8250_force_idle(p);
>> - writel(value, p->membase + (UART_LCR << p->regshift));
>> + d->serial_out(value,
>> + p->membase + (UART_LCR << p->regshift));
>> }
>.. The way I see it, this is the only place where you would really need the new private serial_in/out hooks, so why not just check the >iotype here and based on that use writeb/writel/iowrite32be or what ever ..
In previous versions (V2) Greg dis-liked using iotype here this is why I added the private serial_in/serial_out
>> static void dw8250_setup_port(struct uart_port *p) {
>> struct uart_8250_port *up = up_to_u8250p(p);
>> + struct dw8250_data *d = p->private_data;
>> u32 reg;
>>
>> /*
>> * If the Component Version Register returns zero, we know that
>> * ADDITIONAL_FEATURES are not enabled. No need to go any further.
>> */
>> - reg = readl(p->membase + DW_UART_UCV);
>> + reg = d->serial_in(p->membase + DW_UART_UCV);
>> if (!reg)
>> return;
>>
>> dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>> (reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>>
>> - reg = readl(p->membase + DW_UART_CPR);
>> + reg = d->serial_in(p->membase + DW_UART_CPR);
>> if (!reg)
>> return;
>.. Here you could as well replace the direct readl calls with serial_port_in calls.
Again, if you meant here for using iotype it was rejected.
>> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device
>> *pdev)
>>
>> err = device_property_read_u32(p->dev, "reg-io-width", &val);
>> if (!err && val == 4) {
>> - p->iotype = UPIO_MEM32;
>> - p->serial_in = dw8250_serial_in32;
>> - p->serial_out = dw8250_serial_out32;
>> + p->iotype = of_device_is_big_endian(p->dev->of_node) ?
>> + UPIO_MEM32BE : UPIO_MEM32;
>If this has to be tied to DT, do this check in dw8250_quirks.
Thanks , I will move this to dw8250_quirks.
>> dw8250_quirks(p, data);
>>
>> + data->serial_in = _dw8250_serial_in32;
>> + data->serial_out = _dw8250_serial_out32;
>I don't think I understand what is going on here? You would never actually even use _dw8250_serial_in32be/out32be, no?
>Maybe I'm misunderstanding something.
This is a default in case where "reg-io-width != 4".
What is the case you see that they are not used? (_dw8250_serial_in32be is used from dw8250_setup_port just few lines below)
-Noam
Hi Noam,
On Thu, Dec 10, 2015 at 07:26:42AM +0000, Noam Camus wrote:
> >From: Heikki Krogerus [mailto:[email protected]]
> >Sent: Wednesday, December 09, 2015 3:20 PM
>
>
> >> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> >> if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> >> return;
> >> dw8250_force_idle(p);
> >> - writel(value, p->membase + (UART_LCR << p->regshift));
> >> + d->serial_out(value,
> >> + p->membase + (UART_LCR << p->regshift));
> >> }
>
> >.. The way I see it, this is the only place where you would really need the
> >new private serial_in/out hooks, so why not just check the >iotype here and
> >based on that use writeb/writel/iowrite32be or what ever ..
>
> In previous versions (V2) Greg dis-liked using iotype here this is why I added
> the private serial_in/serial_out
I couldn't find that comment in the thread? All he said in his
comment for this was that you should use real if condition instead of
the ternary operator you had there (condition ? true : false).
Why would it not be acceptable? If it would really not be acceptable
(which I don't believe) then you should simply duplicate the lcr
checking to dw8250_seria_out32be like it is done now in
dw8250_serial_out and dw8250_serial_out32, but there still is no need
for the private serial_in/out hooks.
I'm attaching a diff that I think would work as a good starting point
for you.
> >> static void dw8250_setup_port(struct uart_port *p) {
> >> struct uart_8250_port *up = up_to_u8250p(p);
> >> + struct dw8250_data *d = p->private_data;
> >> u32 reg;
> >>
> >> /*
> >> * If the Component Version Register returns zero, we know that
> >> * ADDITIONAL_FEATURES are not enabled. No need to go any further.
> >> */
> >> - reg = readl(p->membase + DW_UART_UCV);
> >> + reg = d->serial_in(p->membase + DW_UART_UCV);
> >> if (!reg)
> >> return;
> >>
> >> dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
> >> (reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
> >>
> >> - reg = readl(p->membase + DW_UART_CPR);
> >> + reg = d->serial_in(p->membase + DW_UART_CPR);
> >> if (!reg)
> >> return;
>
> >.. Here you could as well replace the direct readl calls with serial_port_in
> >calls.
> Again, if you meant here for using iotype it was rejected.
No, I mean instead of d->serial_in you could just use p->serial_in
here (which is the same as calling serial_port_in()).
> >> @@ -390,9 +434,19 @@ static int dw8250_probe(struct platform_device
> >> *pdev)
> >>
> >> err = device_property_read_u32(p->dev, "reg-io-width", &val);
> >> if (!err && val == 4) {
> >> - p->iotype = UPIO_MEM32;
> >> - p->serial_in = dw8250_serial_in32;
> >> - p->serial_out = dw8250_serial_out32;
> >> + p->iotype = of_device_is_big_endian(p->dev->of_node) ?
> >> + UPIO_MEM32BE : UPIO_MEM32;
>
> >If this has to be tied to DT, do this check in dw8250_quirks.
> Thanks , I will move this to dw8250_quirks.
>
> >> dw8250_quirks(p, data);
> >>
> >> + data->serial_in = _dw8250_serial_in32;
> >> + data->serial_out = _dw8250_serial_out32;
>
> >I don't think I understand what is going on here? You would never actually
> >even use _dw8250_serial_in32be/out32be, no?
>
> >Maybe I'm misunderstanding something.
> This is a default in case where "reg-io-width != 4".
> What is the case you see that they are not used? (_dw8250_serial_in32be is
> used from dw8250_setup_port just few lines below)
But dw8250_setup_port will call data->serial_in hook based on this
patch, which will now be pointing to _dw8250_serial_in32, not
_dw8250_serial_in32be?
Thanks,
--
heikki
On Thu, Dec 10, 2015 at 11:31:04AM +0200, Heikki Krogerus wrote:
> Hi Noam,
>
> On Thu, Dec 10, 2015 at 07:26:42AM +0000, Noam Camus wrote:
> > >From: Heikki Krogerus [mailto:[email protected]]
> > >Sent: Wednesday, December 09, 2015 3:20 PM
> >
> >
> > >> @@ -171,7 +174,8 @@ static void dw8250_serial_out32(struct uart_port *p, int offset, int value)
> > >> if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
> > >> return;
> > >> dw8250_force_idle(p);
> > >> - writel(value, p->membase + (UART_LCR << p->regshift));
> > >> + d->serial_out(value,
> > >> + p->membase + (UART_LCR << p->regshift));
> > >> }
> >
> > >.. The way I see it, this is the only place where you would really need the
> > >new private serial_in/out hooks, so why not just check the >iotype here and
> > >based on that use writeb/writel/iowrite32be or what ever ..
> >
> > In previous versions (V2) Greg dis-liked using iotype here this is why I added
> > the private serial_in/serial_out
>
> I couldn't find that comment in the thread? All he said in his
> comment for this was that you should use real if condition instead of
> the ternary operator you had there (condition ? true : false).
>
> Why would it not be acceptable? If it would really not be acceptable
> (which I don't believe) then you should simply duplicate the lcr
> checking to dw8250_seria_out32be like it is done now in
> dw8250_serial_out and dw8250_serial_out32, but there still is no need
> for the private serial_in/out hooks.
>
> I'm attaching a diff that I think would work as a good starting point
> for you.
Now actually attaching the diff :).
--
heikki