Re: [PATCH 10/11] usb: host: xhci-tegra: Add Tegra234 XHCI support

[Thierry Reding <thierry.reding@gmail.com>]

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

On Mon, Oct 24, 2022 at 03:41:27PM +0800, Wayne Chang wrote:
> From: Sing-Han Chen <singhanc@nvidia.com>
> 
> This change adds Tegra234 XUSB host mode controller support.
> 
> In Tegra234, some of the registers have moved to bar2 space.
> The new soc variable has_bar2 indicates the chip with bar2
> area. This patch adds new reg helper to let the driver reuse
> the same code for those chips with bar2 support.
> 
> The new soc variables has_ifr indicates the chip with IFR FW
> loading support. IFR registers would be configured in
> MB1, and FW loading will be triggered in MB2.
> 
> Signed-off-by: Sing-Han Chen <singhanc@nvidia.com>
> Co-developed-by: Wayne Chang <waynec@nvidia.com>
> Signed-off-by: Wayne Chang <waynec@nvidia.com>
> ---
>  drivers/usb/host/xhci-tegra.c | 277 +++++++++++++++++++++++++++++-----
>  1 file changed, 237 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c
> index bdb776553826..86036eeece43 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -44,6 +44,9 @@
>  #define XUSB_CFG_4				0x010
>  #define  XUSB_BASE_ADDR_SHIFT			15
>  #define  XUSB_BASE_ADDR_MASK			0x1ffff
> +#define XUSB_CFG_7				0x01c
> +#define  XUSB_BASE2_ADDR_SHIFT			16
> +#define  XUSB_BASE2_ADDR_MASK			0xffff
>  #define XUSB_CFG_16				0x040
>  #define XUSB_CFG_24				0x060
>  #define XUSB_CFG_AXI_CFG			0x0f8
> @@ -75,6 +78,20 @@
>  #define  MBOX_SMI_INTR_FW_HANG			BIT(1)
>  #define  MBOX_SMI_INTR_EN			BIT(3)
>  
> +/* BAR2 registers */
> +#define XUSB_BAR2_ARU_MBOX_CMD			0x004
> +#define XUSB_BAR2_ARU_MBOX_DATA_IN		0x008
> +#define XUSB_BAR2_ARU_MBOX_DATA_OUT		0x00c
> +#define XUSB_BAR2_ARU_MBOX_OWNER		0x010
> +#define XUSB_BAR2_ARU_SMI_INTR			0x014
> +#define XUSB_BAR2_ARU_SMI_ARU_FW_SCRATCH_DATA0	0x01c
> +#define XUSB_BAR2_ARU_IFRDMA_CFG0		0x0e0
> +#define XUSB_BAR2_ARU_IFRDMA_CFG1		0x0e4
> +#define XUSB_BAR2_ARU_IFRDMA_STREAMID_FIELD	0x0e8
> +#define XUSB_BAR2_ARU_C11_CSBRANGE		0x9c
> +#define XUSB_BAR2_ARU_FW_SCRATCH		0x1000
> +#define XUSB_BAR2_CSB_BASE_ADDR			0x2000
> +
>  /* IPFS registers */
>  #define IPFS_XUSB_HOST_MSI_BAR_SZ_0		0x0c0
>  #define IPFS_XUSB_HOST_MSI_AXI_BAR_ST_0		0x0c4
> @@ -111,6 +128,9 @@
>  #define  IMFILLRNG1_TAG_HI_SHIFT		16
>  #define XUSB_FALC_IMFILLCTL			0x158
>  
> +/* CSB ARU  registers */

Weird double-space between "ARU" and "registers".

> +#define XUSB_CSB_ARU_SCRATCH0			0x100100
> +
>  /* MP CSB registers */
>  #define XUSB_CSB_MP_ILOAD_ATTR			0x101a00
>  #define XUSB_CSB_MP_ILOAD_BASE_LO		0x101a04
> @@ -131,6 +151,9 @@
>  
>  #define IMEM_BLOCK_SIZE				256
>  
> +#define FW_IOCTL_TYPE_SHIFT             (24)

This should use tabs for spacing, like all the other definitions. Also,
no need to wrap literal integers in parentheses.

> +#define FW_IOCTL_CFGTBL_READ		(17)

No need for the parentheses.

> +
>  struct tegra_xusb_fw_header {
>  	__le32 boot_loadaddr_in_imem;
>  	__le32 boot_codedfi_offset;
> @@ -175,6 +198,7 @@ struct tegra_xusb_mbox_regs {
>  	u16 data_in;
>  	u16 data_out;
>  	u16 owner;
> +	u16 smi_intr;
>  };
>  
>  struct tegra_xusb_context_soc {
> @@ -189,6 +213,7 @@ struct tegra_xusb_context_soc {
>  	} fpci;
>  };
>  
> +struct tegra_xusb_soc_ops;

Probably better to move the definition of the structure here and instead
predeclare struct tegra_xusb.

>  struct tegra_xusb_soc {
>  	const char *firmware;
>  	const char * const *supply_names;
> @@ -205,11 +230,15 @@ struct tegra_xusb_soc {
>  	} ports;
>  
>  	struct tegra_xusb_mbox_regs mbox;
> +	struct tegra_xusb_soc_ops *ops;

const please.

>  
>  	bool scale_ss_clock;
>  	bool has_ipfs;
>  	bool lpm_support;
>  	bool otg_reset_sspi;
> +
> +	bool has_bar2;
> +	bool has_ifr;
>  };
>  
>  struct tegra_xusb_context {
> @@ -230,6 +259,8 @@ struct tegra_xusb {
>  
>  	void __iomem *ipfs_base;
>  	void __iomem *fpci_base;
> +	void __iomem *bar2_base;
> +	resource_size_t bar2_start;

Maybe just store struct resource *bar2, here.

[...]
> @@ -664,6 +754,7 @@ static void tegra_xusb_mbox_handle(struct tegra_xusb *tegra,
>  static irqreturn_t tegra_xusb_mbox_thread(int irq, void *data)
>  {
>  	struct tegra_xusb *tegra = data;
> +	struct tegra_xusb_soc_ops *ops = tegra->soc->ops;

const

> @@ -709,6 +800,15 @@ static void tegra_xusb_config(struct tegra_xusb *tegra)
>  	value |= regs & (XUSB_BASE_ADDR_MASK << XUSB_BASE_ADDR_SHIFT);
>  	fpci_writel(tegra, value, XUSB_CFG_4);
>  
> +	/* Program BAR2 space */
> +	if (tegra->soc->has_bar2) {

You could make this depend on tegra->bar2 if you make the change above.

> +		value = fpci_readl(tegra, XUSB_CFG_7);
> +		value &= ~(XUSB_BASE2_ADDR_MASK << XUSB_BASE2_ADDR_SHIFT);
> +		value |= tegra->bar2_start &
> +			(XUSB_BASE2_ADDR_MASK << XUSB_BASE2_ADDR_SHIFT);
> +		fpci_writel(tegra, value, XUSB_CFG_7);
> +	}
> +
>  	usleep_range(100, 200);
>  
>  	/* Enable bus master */
> @@ -881,21 +981,36 @@ static int tegra_xusb_request_firmware(struct tegra_xusb *tegra)
>  	return 0;
>  }
>  
> +static int tegra_xusb_wait_for_falcon(struct tegra_xusb *tegra)
> +{
> +	struct xhci_cap_regs __iomem *cap_regs;
> +	struct xhci_op_regs __iomem *op_regs;
> +	int ret;
> +	u32 val;

Use "value" for consistency with the rest of the driver.

> +
> +	cap_regs = tegra->regs;
> +	op_regs = tegra->regs + HC_LENGTH(readl(&cap_regs->hc_capbase));
> +
> +	ret = readl_poll_timeout(&op_regs->status, val, !(val & STS_CNR), 1000, 200000);
> +
> +	if (ret)
> +		dev_err(tegra->dev, "XHCI Controller not ready. Falcon state: 0x%x\n",
> +			csb_readl(tegra, XUSB_FALC_CPUCTL));
> +
> +	return ret;
> +}

This refactoring could be a separate patch. It makes the rest of the
changes harder to review. Not necessarily something that needs to be
addressed, though.

> +
>  static int tegra_xusb_load_firmware(struct tegra_xusb *tegra)
>  {
>  	unsigned int code_tag_blocks, code_size_blocks, code_blocks;
> -	struct xhci_cap_regs __iomem *cap = tegra->regs;
>  	struct tegra_xusb_fw_header *header;
>  	struct device *dev = tegra->dev;
> -	struct xhci_op_regs __iomem *op;
> -	unsigned long timeout;
>  	time64_t timestamp;
>  	u64 address;
>  	u32 value;
>  	int err;
>  
>  	header = (struct tegra_xusb_fw_header *)tegra->fw.virt;
> -	op = tegra->regs + HC_LENGTH(readl(&cap->hc_capbase));
>  
>  	if (csb_readl(tegra, XUSB_CSB_MP_ILOAD_BASE_LO) != 0) {
>  		dev_info(dev, "Firmware already loaded, Falcon state %#x\n",
> @@ -968,26 +1083,43 @@ static int tegra_xusb_load_firmware(struct tegra_xusb *tegra)
>  	/* Boot Falcon CPU and wait for USBSTS_CNR to get cleared. */
>  	csb_writel(tegra, CPUCTL_STARTCPU, XUSB_FALC_CPUCTL);
>  
> -	timeout = jiffies + msecs_to_jiffies(200);
> +	if (tegra_xusb_wait_for_falcon(tegra))
> +		return -EIO;
>  
> -	do {
> -		value = readl(&op->status);
> -		if ((value & STS_CNR) == 0)
> -			break;
> +	timestamp = le32_to_cpu(header->fwimg_created_time);
>  
> -		usleep_range(1000, 2000);
> -	} while (time_is_after_jiffies(timeout));
> +	dev_info(dev, "Firmware timestamp: %ptTs UTC\n", &timestamp);
> +
> +	return 0;
> +}
>  
> -	value = readl(&op->status);
> -	if (value & STS_CNR) {
> -		value = csb_readl(tegra, XUSB_FALC_CPUCTL);
> -		dev_err(dev, "XHCI controller not read: %#010x\n", value);
> +static u32 tegra_xusb_read_firmware_header(struct tegra_xusb *tegra, u32 offset)
> +{
> +	/*
> +	 * We only accept reading the firmware config table
> +	 * The offset should not exceed the fw header structure
> +	 */
> +	if (offset >= sizeof(struct tegra_xusb_fw_header))
> +		return 0;

You technically still allow reading 3 bytes past the header structure.
Or does the firmware's CFGTL_READ IOCTL mask out the lower 2 bits of the
offset?

> +
> +	bar2_writel(tegra, (FW_IOCTL_CFGTBL_READ << FW_IOCTL_TYPE_SHIFT) | offset,
> +			XUSB_BAR2_ARU_FW_SCRATCH);
> +	return bar2_readl(tegra, XUSB_BAR2_ARU_SMI_ARU_FW_SCRATCH_DATA0);
> +}
> +
> +static int tegra_xusb_init_ifr_firmware(struct tegra_xusb *tegra)
> +{
> +	time64_t timestamp;
> +
> +	if (tegra_xusb_wait_for_falcon(tegra))
>  		return -EIO;
> -	}
>  
> -	timestamp = le32_to_cpu(header->fwimg_created_time);
> +#define offsetof_32(X, Y) ((u8)(offsetof(X, Y) / sizeof(__le32)))
> +	timestamp = tegra_xusb_read_firmware_header(tegra,
> +			offsetof_32(struct tegra_xusb_fw_header,
> +				fwimg_created_time) << 2);
>  
> -	dev_info(dev, "Firmware timestamp: %ptTs UTC\n", &timestamp);
> +	dev_info(tegra->dev, "Firmware timestamp: %ptTs UTC\n", &timestamp);
>  
>  	return 0;
>  }
> @@ -1403,7 +1535,7 @@ static int tegra_xusb_probe(struct platform_device *pdev)
>  	struct of_phandle_args args;
>  	struct tegra_xusb *tegra;
>  	struct device_node *np;
> -	struct resource *regs;
> +	struct resource *res, *regs;
>  	struct xhci_hcd *xhci;
>  	unsigned int i, j, k;
>  	struct phy *phy;
> @@ -1435,6 +1567,11 @@ static int tegra_xusb_probe(struct platform_device *pdev)
>  		tegra->ipfs_base = devm_platform_ioremap_resource(pdev, 2);
>  		if (IS_ERR(tegra->ipfs_base))
>  			return PTR_ERR(tegra->ipfs_base);
> +	} else if (tegra->soc->has_bar2) {
> +		tegra->bar2_base = devm_platform_get_and_ioremap_resource(pdev, 2, &res);

If you store struct resource *bar2 in tegra, you can pass &tegra->bar2
here and ...

> +		if (IS_ERR(tegra->bar2_base))
> +			return PTR_ERR(tegra->bar2_base);
> +		tegra->bar2_start = res->start;

... skip this.

>  	}
>  
>  	tegra->xhci_irq = platform_get_irq(pdev, 0);
> @@ -1651,10 +1788,13 @@ static int tegra_xusb_probe(struct platform_device *pdev)
>  		goto disable_phy;
>  	}
>  
> -	err = tegra_xusb_request_firmware(tegra);
> -	if (err < 0) {
> -		dev_err(&pdev->dev, "failed to request firmware: %d\n", err);
> -		goto disable_phy;
> +	if (!tegra->soc->has_ifr) {
> +		err = tegra_xusb_request_firmware(tegra);
> +		if (err < 0) {
> +			dev_err(&pdev->dev,
> +				"failed to request firmware: %d\n", err);
> +			goto disable_phy;
> +		}
>  	}
>  
>  	err = tegra_xusb_unpowergate_partitions(tegra);
> @@ -1663,7 +1803,10 @@ static int tegra_xusb_probe(struct platform_device *pdev)
>  
>  	tegra_xusb_config(tegra);
>  
> -	err = tegra_xusb_load_firmware(tegra);
> +	if (tegra->soc->has_ifr)
> +		err = tegra_xusb_init_ifr_firmware(tegra);
> +	else
> +		err = tegra_xusb_load_firmware(tegra);
>  	if (err < 0) {
>  		dev_err(&pdev->dev, "failed to load firmware: %d\n", err);
>  		goto powergate;
> @@ -2070,7 +2213,10 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime)
>  	tegra_xusb_config(tegra);
>  	tegra_xusb_restore_context(tegra);
>  
> -	err = tegra_xusb_load_firmware(tegra);
> +	if (tegra->soc->has_ifr)
> +		err = tegra_xusb_init_ifr_firmware(tegra);
> +	else
> +		err = tegra_xusb_load_firmware(tegra);

Might be worth extracting this into a new function since you use this
twice now.

>  	if (err < 0) {
>  		dev_err(tegra->dev, "failed to load firmware: %d\n", err);
>  		goto disable_phy;
> @@ -2271,6 +2417,13 @@ static const struct tegra_xusb_context_soc tegra124_xusb_context = {
>  	},
>  };
>  
> +static struct tegra_xusb_soc_ops tegra124_ops = {

const

> +	.mbox_reg_readl = &fpci_readl,
> +	.mbox_reg_writel = &fpci_writel,
> +	.csb_reg_readl = &fpci_csb_readl,
> +	.csb_reg_writel = &fpci_csb_writel,
> +};
> +
>  static const struct tegra_xusb_soc tegra124_soc = {
>  	.firmware = "nvidia/tegra124/xusb.bin",
>  	.supply_names = tegra124_supply_names,
> @@ -2286,11 +2439,13 @@ static const struct tegra_xusb_soc tegra124_soc = {
>  	.scale_ss_clock = true,
>  	.has_ipfs = true,
>  	.otg_reset_sspi = false,
> +	.ops = &tegra124_ops,
>  	.mbox = {
>  		.cmd = 0xe4,
>  		.data_in = 0xe8,
>  		.data_out = 0xec,
>  		.owner = 0xf0,
> +		.smi_intr = XUSB_CFG_ARU_SMI_INTR,
>  	},
>  };
>  MODULE_FIRMWARE("nvidia/tegra124/xusb.bin");
> @@ -2322,11 +2477,13 @@ static const struct tegra_xusb_soc tegra210_soc = {
>  	.scale_ss_clock = false,
>  	.has_ipfs = true,
>  	.otg_reset_sspi = true,
> +	.ops = &tegra124_ops,
>  	.mbox = {
>  		.cmd = 0xe4,
>  		.data_in = 0xe8,
>  		.data_out = 0xec,
>  		.owner = 0xf0,
> +		.smi_intr = XUSB_CFG_ARU_SMI_INTR,
>  	},
>  };
>  MODULE_FIRMWARE("nvidia/tegra210/xusb.bin");
> @@ -2363,11 +2520,13 @@ static const struct tegra_xusb_soc tegra186_soc = {
>  	.scale_ss_clock = false,
>  	.has_ipfs = false,
>  	.otg_reset_sspi = false,
> +	.ops = &tegra124_ops,
>  	.mbox = {
>  		.cmd = 0xe4,
>  		.data_in = 0xe8,
>  		.data_out = 0xec,
>  		.owner = 0xf0,
> +		.smi_intr = XUSB_CFG_ARU_SMI_INTR,
>  	},
>  	.lpm_support = true,
>  };
> @@ -2394,21 +2553,59 @@ static const struct tegra_xusb_soc tegra194_soc = {
>  	.scale_ss_clock = false,
>  	.has_ipfs = false,
>  	.otg_reset_sspi = false,
> +	.ops = &tegra124_ops,
>  	.mbox = {
>  		.cmd = 0x68,
>  		.data_in = 0x6c,
>  		.data_out = 0x70,
>  		.owner = 0x74,
> +		.smi_intr = XUSB_CFG_ARU_SMI_INTR,
>  	},
>  	.lpm_support = true,
>  };
>  MODULE_FIRMWARE("nvidia/tegra194/xusb.bin");
>  
> +static struct tegra_xusb_soc_ops tegra234_ops = {

const

> +	.mbox_reg_readl = &bar2_readl,
> +	.mbox_reg_writel = &bar2_writel,
> +	.csb_reg_readl = &bar2_csb_readl,
> +	.csb_reg_writel = &bar2_csb_writel,
> +};
> +
> +static const struct tegra_xusb_soc tegra234_soc = {
> +	.firmware = "nvidia/tegra234/xusb.bin",
> +	.supply_names = tegra194_supply_names,
> +	.num_supplies = ARRAY_SIZE(tegra194_supply_names),
> +	.phy_types = tegra194_phy_types,
> +	.num_types = ARRAY_SIZE(tegra194_phy_types),
> +	.context = &tegra186_xusb_context,
> +	.ports = {
> +		.usb3 = { .offset = 0, .count = 4, },
> +		.usb2 = { .offset = 4, .count = 4, },
> +	},
> +	.scale_ss_clock = false,
> +	.has_ipfs = false,
> +	.otg_reset_sspi = false,
> +	.ops = &tegra234_ops,
> +	.mbox = {
> +		.cmd = XUSB_BAR2_ARU_MBOX_CMD,
> +		.data_in = XUSB_BAR2_ARU_MBOX_DATA_IN,
> +		.data_out = XUSB_BAR2_ARU_MBOX_DATA_OUT,
> +		.owner = XUSB_BAR2_ARU_MBOX_OWNER,
> +		.smi_intr = XUSB_BAR2_ARU_SMI_INTR,
> +	},
> +	.lpm_support = true,
> +	.has_bar2 = true,
> +	.has_ifr = true,
> +};
> +MODULE_FIRMWARE("nvidia/tegra234/xusb.bin");

Can you prepare a patch to add this firmware to the linux-firmware
repository? I don't see it there yet.

Thierry

> +
>  static const struct of_device_id tegra_xusb_of_match[] = {
>  	{ .compatible = "nvidia,tegra124-xusb", .data = &tegra124_soc },
>  	{ .compatible = "nvidia,tegra210-xusb", .data = &tegra210_soc },
>  	{ .compatible = "nvidia,tegra186-xusb", .data = &tegra186_soc },
>  	{ .compatible = "nvidia,tegra194-xusb", .data = &tegra194_soc },
> +	{ .compatible = "nvidia,tegra234-xusb", .data = &tegra234_soc },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, tegra_xusb_of_match);
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]