Re: [PATCH v4 6/7] serial: exar: add CTI specific setup code

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4014 bytes --]

On Thu, 18 Apr 2024, Parker Newman wrote:
> On Thu, 18 Apr 2024 16:20:15 +0300 (EEST)
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > On Wed, 17 Apr 2024, Parker Newman wrote:
> > > From: Parker Newman <pnewman@connecttech.com>
> > > 
> > > This is a large patch but is only additions. All changes and removals
> > > are made in previous patches in this series.
> > > 
> > > - Add CTI board_init and port setup functions for each UART type
> > > - Add CTI_EXAR_DEVICE() and CTI_PCI_DEVICE() macros
> > > - Add support for reading a word from the Exar EEPROM.
> > > - Add support for configuring and setting a single MPIO
> > > - Add various helper functions for CTI boards.
> > > - Add osc_freq to struct exar8250
> > > 
> > > Signed-off-by: Parker Newman <pnewman@connecttech.com>

> > > @@ -192,11 +252,201 @@ struct exar8250_board {
> > > 
> > >  struct exar8250 {
> > >  	unsigned int		nr;
> > > +	unsigned int		osc_freq;
> > >  	struct exar8250_board	*board;
> > >  	void __iomem		*virt;
> > >  	int			line[];
> > >  };
> > > 
> > > +static inline void exar_write_reg(struct exar8250 *priv,
> > > +				unsigned int reg, u8 value)
> > > +{
> > > +	writeb(value, priv->virt + reg);
> > > +}
> > > +
> > > +static inline u8 exar_read_reg(struct exar8250 *priv, unsigned int reg)
> > > +{
> > > +	return readb(priv->virt + reg);
> > > +}  
> > 
> > I tried to understand what is going on with this priv->virt in 8250_exar 
> > in general and why it exists but I failed. It seems to BAR0 is mapped 
> > there but also serial8250_pci_setup_port() does map the same BAR and 
> > sets it up into the usual place in membase.
> > 
> 
> Exar PCI/PCIe UARTs have global configuration registers from 0x80-0x9B.
> These registers are for reading the EEPROM, configuring the MPIO, etc.
> As these registers are only at 0x80, and not port specific, the driver maps
> BAR0 to priv->virt for accessing them. 

Okay, thanks for explaining. The naming & lack of comments wasn't exactly 
making it easy to follow this bit (this is not your fault in anyway but 
a pre-existing problem in the driver's code).

I've a follow up question now that it's confirmed they're different, 
see below...

> > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 128);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 128);  
> > 
> > Unnecessary parenthesis.

> > > +	exar_write_reg(priv, (offset + UART_EXAR_8XMODE), 0x00);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_FCTR), UART_FCTR_EXAR_TRGD);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_TXTRG), 32);
> > > +	exar_write_reg(priv, (offset + UART_EXAR_RXTRG), 32);  
> > 
> > Unnecessary parenthesis.
> > 
> 
> I will fix these in my cleanup patches. 

Based on the wording in your response, I'm not sure you got this right. It 
is code you're adding in this patch so the parenthesis should be removed 
from this change so they never appear in the commit history.

> > I recommend you add a helper for this as it is repeated twice. Are the 
> > values 32 and 128 literal or do they have some specific meaning? If the 
> > latter case, they should be using named defines (this likely applies to 
> > the existing trigger code in the driver too).
> > 
> > 
> 
> They are the FIFO trigger levels so they are literally 128 and 32. 

Okay, no problem then if its 128 characters and 32 characters.

> These 4 writes come from Exar's out-of-tree driver and are in 
> pci_xr17v35x_setup() and some other vendor specific functions. 
> 
> I am not sure why/if these are needed. 

...So the follow-up question. I see the existing code in 
pci_fastcom335_setup() and pci_xr17v35x_setup() writes into membase 
based address but your code uses exar_write_reg() which is priv->virt 
based. Is this difference intentional?

-- 
 i.