Message-ID: <907dade8-0e11-4d99-acb5-2d35ba729932@kernel.org>
Date: Sat, 18 Jan 2025 09:24:42 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Kevin Chen <kevin_chen@...eedtech.com>, joel@....id.au,
 andrew@...econstruct.com.au, linux-arm-kernel@...ts.infradead.org,
 linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
 elbadrym@...gle.com
Subject: Re: [PATCH v0 3/3] soc: aspeed: lpc-pcc: Add PCC controller support

On 17/01/2025 10:52, Kevin Chen wrote:
> Add LPC PCC controller driver to support POST code capture.
> 
> Signed-off-by: Kevin Chen <kevin_chen@...eedtech.com>
> ---


This patch was never tested with your DTS and bindings. Please do not
send untested code.

> +
> +/*
> + * A2600-15 AP note
> + *
> + * SW workaround to prevent generating Non-Fatal-Error (NFE)
> + * eSPI response when PCC is used for port I/O byte snooping
> + * over eSPI.
> + */
> +static int aspeed_a2600_15(struct aspeed_pcc *pcc, struct device *dev)
> +{
> +	struct device_node *np;
> +	u32 hicrb_en;
> +
> +	/* abort if snoop is enabled */
> +	np = of_find_compatible_node(dev->parent->of_node, NULL, "aspeed,ast2600-lpc-snoop");

No, don't sprinkle compatible to epxress relationship. Use proper
phandles or parent-child relationship.

> +	if (np) {
> +		if (of_device_is_available(np)) {
> +			dev_err(dev, "A2600-15 should be applied with snoop disabled\n");
> +			return -EPERM;
> +		}
> +	}
> +
> +	/* abort if port is not 4-bytes continuous range */
> +	if (pcc->port_xbits != 0x3) {
> +		dev_err(dev, "A2600-15 should be applied on 4-bytes continuous I/O address range\n");
> +		return -EINVAL;
> +	}
> +
> +	/* set SNPWADR of snoop device */
> +	regmap_write(pcc->regmap, SNPWADR, pcc->port | ((pcc->port + 2) << 16));
> +
> +	/* set HICRB[15:14]=11b to enable ACCEPT response for SNPWADR */
> +	hicrb_en = HICRB_ENSNP0D | HICRB_ENSNP1D;
> +	regmap_update_bits(pcc->regmap, HICRB, hicrb_en, hicrb_en);
> +
> +	/* set HICR6[19] to extend SNPWADR to 2x range */
> +	regmap_update_bits(pcc->regmap, HICR6, HICR6_EN2BMODE, HICR6_EN2BMODE);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pcc_enable(struct aspeed_pcc *pcc, struct device *dev)
> +{
> +	int rc;
> +
> +	if (pcc->a2600_15) {
> +		rc = aspeed_a2600_15(pcc, dev);
> +		if (rc)
> +			return rc;
> +	}
> +
> +	/* record mode */
> +	regmap_update_bits(pcc->regmap, PCCR0,
> +			   PCCR0_MODE_SEL_MASK,
> +			   pcc->rec_mode << PCCR0_MODE_SEL_SHIFT);
> +
> +	/* port address */
> +	regmap_update_bits(pcc->regmap, PCCR1,
> +			   PCCR1_BASE_ADDR_MASK,
> +			   pcc->port << PCCR1_BASE_ADDR_SHIFT);
> +
> +	/* port address high bits selection or parser control */
> +	regmap_update_bits(pcc->regmap, PCCR0,
> +			   PCCR0_ADDR_SEL_MASK,
> +			   pcc->port_hbits_select << PCCR0_ADDR_SEL_SHIFT);
> +
> +	/* port address dont care bits */
> +	regmap_update_bits(pcc->regmap, PCCR1,
> +			   PCCR1_DONT_CARE_BITS_MASK,
> +			   pcc->port_xbits << PCCR1_DONT_CARE_BITS_SHIFT);
> +
> +	/* set DMA ring buffer size and enable interrupts */
> +	if (pcc->dma_mode) {
> +		regmap_write(pcc->regmap, PCCR4, pcc->dma.addr & 0xffffffff);
> +		regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_ADDRH_MASK,
> +				   (pcc->dma.addr >> 32) << PCCR5_DMA_ADDRH_SHIFT);
> +		regmap_update_bits(pcc->regmap, PCCR5, PCCR5_DMA_LEN_MASK,
> +				   (pcc->dma.size / 4) << PCCR5_DMA_LEN_SHIFT);
> +		regmap_update_bits(pcc->regmap, PCCR0,
> +				   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE,
> +				   PCCR0_EN_DMA_INT | PCCR0_EN_DMA_MODE);
> +	} else {
> +		regmap_update_bits(pcc->regmap, PCCR0, PCCR0_RX_TRIG_LVL_MASK,
> +				   PCC_FIFO_THR_4_EIGHTH << PCCR0_RX_TRIG_LVL_SHIFT);
> +		regmap_update_bits(pcc->regmap, PCCR0,
> +				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT,
> +				   PCCR0_EN_RX_TMOUT_INT | PCCR0_EN_RX_AVAIL_INT);
> +	}
> +
> +	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN, PCCR0_EN);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pcc_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	struct aspeed_pcc *pcc;
> +	struct device *dev = &pdev->dev;
> +	uint32_t fifo_size = PAGE_SIZE;
> +
> +	pcc = devm_kzalloc(&pdev->dev, sizeof(*pcc), GFP_KERNEL);

Why not dev?

> +	if (!pcc)
> +		return -ENOMEM;
> +
> +	pcc->dev = dev;
> +	rc = of_property_read_u32(dev->of_node, "port-addr", &pcc->port);

Nope

> +	if (rc) {
> +		dev_err(dev, "cannot get port address\n");
> +		return -ENODEV;
> +	}
> +
> +	/* optional, by default: 0 -> 1-Byte mode */
> +	of_property_read_u32(dev->of_node, "rec-mode", &pcc->rec_mode);

Nope

> +	if (!is_valid_rec_mode(pcc->rec_mode)) {
> +		dev_err(dev, "invalid record mode: %u\n",
> +			pcc->rec_mode);
> +		return -EINVAL;
> +	}
> +
> +	/* optional, by default: 0 -> no don't care bits */
> +	of_property_read_u32(dev->of_node, "port-addr-xbits", &pcc->port_xbits);

Nope

> +
> +	/*
> +	 * optional, by default: 0 -> no high address bits
> +	 *
> +	 * Note that when record mode is set to 1-Byte, this
> +	 * property is ignored and the corresponding HW bits
> +	 * behave as read/write cycle parser control with the
> +	 * value set to 0b11
> +	 */
> +	if (pcc->rec_mode) {
> +		of_property_read_u32(dev->of_node, "port-addr-hbits-select",

Nope, none of these properties exist.

> +				     &pcc->port_hbits_select);
> +		if (!is_valid_high_bits_select(pcc->port_hbits_select)) {
> +			dev_err(dev, "invalid high address bits selection: %u\n",
> +				pcc->port_hbits_select);
> +			return -EINVAL;
> +		}
> +	} else {
> +		pcc->port_hbits_select = 0x3;
> +	}
> +
> +	/* AP note A2600-15 */
> +	pcc->a2600_15 = of_property_read_bool(dev->of_node, "A2600-15");

NAK even more.

> +	if (pcc->a2600_15)
> +		dev_info(dev, "A2600-15 AP note patch is selected\n");
> +> +	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> +	if (rc) {
> +		dev_err(dev, "cannot set 64-bits DMA mask\n");
> +		return rc;
> +	}
> +
> +	pcc->dma_mode = of_property_read_bool(dev->of_node, "dma-mode");

NAK, drop

> +	if (pcc->dma_mode) {
> +		pcc->dma.size = PCC_DMA_BUFSZ;
> +		pcc->dma.virt = dmam_alloc_coherent(dev,
> +						    pcc->dma.size,
> +						    &pcc->dma.addr,
> +						    GFP_KERNEL);
> +		if (!pcc->dma.virt) {
> +			dev_err(dev, "cannot allocate DMA buffer\n");
> +			return -ENOMEM;
> +		}
> +
> +		fifo_size = roundup(pcc->dma.size, PAGE_SIZE);
> +	}
> +
> +	rc = kfifo_alloc(&pcc->fifo, fifo_size, GFP_KERNEL);
> +	if (rc) {
> +		dev_err(dev, "cannot allocate kFIFO\n");

Run coccinelle on your code first.

> +		return -ENOMEM;
> +	}
> +
> +	pcc->regmap = syscon_node_to_regmap(pdev->dev.parent->of_node);
> +	if (IS_ERR(pcc->regmap)) {
> +		dev_err(dev, "cannot map register\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Disable PCC and DMA Mode for safety */
> +	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_EN |  PCCR0_EN_DMA_MODE, 0);
> +
> +	/* Clear Rx FIFO. */
> +	regmap_update_bits(pcc->regmap, PCCR0, PCCR0_CLR_RX_FIFO, 1);
> +
> +	/* Clear All interrupts status. */
> +	regmap_write(pcc->regmap, PCCR2,
> +		     PCCR2_INT_STATUS_RX_OVER | PCCR2_INT_STATUS_DMA_DONE |
> +		     PCCR2_INT_STATUS_PATTERN_A | PCCR2_INT_STATUS_PATTERN_B);
> +
> +	pcc->irq = platform_get_irq(pdev, 0);
> +	if (pcc->irq < 0) {
> +		dev_err(dev, "cannot get IRQ\n");
> +		rc = -ENODEV;
> +		goto err_free_kfifo;
> +	}
> +
> +	rc = devm_request_irq(dev, pcc->irq, aspeed_pcc_isr, 0, DEVICE_NAME, pcc);
> +	if (rc < 0) {
> +		dev_err(dev, "cannot request IRQ handler\n");
> +		goto err_free_kfifo;
> +	}
> +
> +	init_waitqueue_head(&pcc->wq);
> +
> +	pcc->mdev_id = ida_alloc(&aspeed_pcc_ida, GFP_KERNEL);
> +	if (pcc->mdev_id < 0) {
> +		dev_err(dev, "cannot allocate ID\n");
> +		return pcc->mdev_id;
> +	}
> +
> +	pcc->mdev.parent = dev;
> +	pcc->mdev.minor = MISC_DYNAMIC_MINOR;
> +	pcc->mdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> +					pcc->mdev_id);
> +	pcc->mdev.fops = &pcc_fops;
> +	rc = misc_register(&pcc->mdev);
> +	if (rc) {
> +		dev_err(dev, "cannot register misc device\n");
> +		goto err_free_kfifo;
> +	}
> +
> +	rc = aspeed_pcc_enable(pcc, dev);
> +	if (rc) {
> +		dev_err(dev, "cannot enable PCC\n");
> +		goto err_dereg_mdev;
> +	}
> +
> +	dev_set_drvdata(&pdev->dev, pcc);
> +
> +	dev_info(dev, "module loaded\n");

Drop such function success messages, everywhere.

Best regards,
Krzysztof

Powered by blists - more mailing lists