Re: [PATCH v4 4/7] serial: 8250_aspeed_vuart: add VUART over PCI

[Jiri Slaby <jirislaby@kernel.org>]

On 08. 07. 26, 17:35, Grégoire Layet wrote:
...
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -32,6 +32,26 @@
>   #define ASPEED_VUART_DEFAULT_SIRQ	4
>   #define ASPEED_VUART_DEFAULT_SIRQ_POLARITY	IRQ_TYPE_LEVEL_LOW
>   
> +#define ASPEED_SCU_SILICON_REVISION_ID			0x04
> +#define AST2600A3_REVISION_ID				0x05030303
> +
> +#define ASPEED_SCUC24			0xC24
> +#define  ASPEED_SCUC24_MSI_ROUTING_MASK			GENMASK(11, 10)
> +#define  ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1		(0x2 << 10)

So is this
FIELD_PREP(ASPEED_SCUC24_MSI_ROUTING_MASK, 2)
?

> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN		BIT(18)
> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN			BIT(17)

Perhaps switch the two (to be in asc order)? And define 14 as _RESERVED 
as well?

> +#define ASPEED_SCU_PCIE_CONF_CTRL	0xC20

Hmm, should these go before 0xC24?

> +#define  SCU_PCIE_CONF_BMC_DEV_EN					BIT(8)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MMIO				BIT(9)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_MSI				BIT(11)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_IRQ				BIT(13)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER	BIT(14)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_E2L				BIT(15)
> +#define  SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE		BIT(21)
> +
> +#define ASPEED_SCU_BMC_DEV_CLASS	0xC68
> +
>   struct aspeed_vuart {
>   	struct device		*dev;
>   	int			line;
> @@ -412,6 +432,63 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
>   	}
>   }
>   
> +static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
> +{
...
> +	if (silicon_revision_id == AST2600A3_REVISION_ID)
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +					ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_MASK,
> +					ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1);
> +	else
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +					/**
> +					 * The bit 14 is reserved in the Datasheet.
> +					 */

If you defined reserved as suggested above, no need for the comment.

> +					ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_MASK,
> +					ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1);
> +	if (rc) {
> +		dev_err(dev, "could not set PCI device 1 MSI interrupt routing\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +

thanks,
-- 
js
suse labs