Re: [PATCH v2 3/7] soc: qcom: add QCOM PBS driver

[Anjelique Melendez]

On 7/26/2023 8:36 AM, Konrad Dybcio wrote:
> On 25.07.2023 21:34, Anjelique Melendez wrote:
>> Add the Qualcomm PBS (Programmable Boot Sequencer) driver. The QCOM PBS
>> driver supports configuring software PBS trigger events through PBS RAM
>> on Qualcomm Technologies, Inc (QTI) PMICs.
>>
>> Signed-off-by: Anjelique Melendez <quic_amelende@xxxxxxxxxxx>
>> ---
> [...]
> 
>> +
>> +	u32			base;
>> +};
>> +
>> +static int qcom_pbs_read(struct pbs_dev *pbs, u32 address, u8 *val)
>> +{
>> +	int ret;
>> +
>> +	address += pbs->base;
> Any reason not to just keep the base address in struct pbs_dev and use
> normal regmap r/w helpers?
> 
> [...]

We created the qcom_pbs read/write helpers to limit code duplication when printing
error messages. 
I am ok with calling regmap_bulk_read/write() and regmap_update_bits()
in code instead of these helpers but wondering how everyone would feel with the error messages
either being duplicated or if error messages should just be removed?

qcom_pbs_read() is called twice, qcom_pbs_write() is called twice(), and 
qcom_pbs_masked_write() is called 6 times.
> 
>> +
>> +static int qcom_pbs_wait_for_ack(struct pbs_dev *pbs, u8 bit_pos)
>> +{
>> +	u16 retries = 2000, delay = 1000;
>> +	int ret;
>> +	u8 val;
>> +
>> +	while (retries--) {
>> +		ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if (val == 0xFF) {
> This should be a constant, not a magic value
ack
> 
>> +			/* PBS error - clear SCRATCH2 register */
>> +			ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			dev_err(pbs->dev, "NACK from PBS for bit %u\n", bit_pos);
>> +			return -EINVAL;
>> +		}
>> +
>> +		if (val & BIT(bit_pos)) {
>> +			dev_dbg(pbs->dev, "PBS sequence for bit %u executed!\n", bit_pos);
>> +			break;
>> +		}
>> +
>> +		usleep_range(delay, delay + 100);
> So worst case scenario this will wait for over 2 seconds?
Yes, worst case scenario will result in waiting for 2.2 seconds
> 
>> +	}
>> +
>> +	if (!retries) {
>> +		dev_err(pbs->dev, "Timeout for PBS ACK/NACK for bit %u\n", bit_pos);
>> +		return -ETIMEDOUT;
>> +	}
>> +
>> +	return 0;
> return 0 instead of break above?
ack
> 
>> +}
>> +
>> +/**
>> + * qcom_pbs_trigger_event() - Trigger the PBS RAM sequence
>> + * @pbs: Pointer to PBS device
>> + * @bitmap: bitmap
>> + *
>> + * This function is used to trigger the PBS RAM sequence to be
>> + * executed by the client driver.
>> + *
>> + * The PBS trigger sequence involves
>> + * 1. setting the PBS sequence bit in PBS_CLIENT_SCRATCH1
>> + * 2. Initiating the SW PBS trigger
>> + * 3. Checking the equivalent bit in PBS_CLIENT_SCRATCH2 for the
>> + *    completion of the sequence.
>> + * 4. If PBS_CLIENT_SCRATCH2 == 0xFF, the PBS sequence failed to execute
>> + *
>> + * Returns: 0 on success, < 0 on failure
>> + */
>> +int qcom_pbs_trigger_event(struct pbs_dev *pbs, u8 bitmap)
>> +{
>> +	u8 val, mask;
>> +	u16 bit_pos;
>> +	int ret;
>> +
>> +	if (!bitmap) {
>> +		dev_err(pbs->dev, "Invalid bitmap passed by client\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (IS_ERR_OR_NULL(pbs))
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&pbs->lock);
>> +	ret = qcom_pbs_read(pbs, PBS_CLIENT_SCRATCH2, &val);
>> +	if (ret < 0)
>> +		goto out;
>> +
>> +	if (val == 0xFF) {
>> +		/* PBS error - clear SCRATCH2 register */
>> +		ret = qcom_pbs_write(pbs, PBS_CLIENT_SCRATCH2, 0);
>> +		if (ret < 0)
>> +			goto out;
>> +	}
>> +
>> +	for (bit_pos = 0; bit_pos < 8; bit_pos++) {
>> +		if (bitmap & BIT(bit_pos)) {
>> +			/*
>> +			 * Clear the PBS sequence bit position in
>> +			 * PBS_CLIENT_SCRATCH2 mask register.
>> +			 */
> Don't think the "in the X register" parts are useful.
ack
> 
>> +			ret = qcom_pbs_masked_write(pbs, PBS_CLIENT_SCRATCH2, BIT(bit_pos), 0);
>> +			if (ret < 0)
>> +				goto error;
>> +
>> +			/*
>> +			 * Set the PBS sequence bit position in
>> +			 * PBS_CLIENT_SCRATCH1 register.
>> +			 */
>> +			val = mask = BIT(bit_pos);
> You're using mask/val for half the function calls..
> Stick with one approach.
ack
> 
> [...]
> 
>> +struct pbs_dev *get_pbs_client_device(struct device *dev)
>> +{
>> +	struct device_node *pbs_dev_node;
>> +	struct platform_device *pdev;
>> +	struct pbs_dev *pbs;
>> +
>> +	pbs_dev_node = of_parse_phandle(dev->of_node, "qcom,pbs", 0);
>> +	if (!pbs_dev_node) {
>> +		dev_err(dev, "Missing qcom,pbs property\n");
>> +		return ERR_PTR(-ENODEV);
>> +	}
>> +
>> +	pdev = of_find_device_by_node(pbs_dev_node);
>> +	if (!pdev) {
>> +		dev_err(dev, "Unable to find PBS dev_node\n");
>> +		pbs = ERR_PTR(-EPROBE_DEFER);
>> +		goto out;
>> +	}
>> +
>> +	pbs = platform_get_drvdata(pdev);
>> +	if (!pbs) {
> This check seems unnecessary, the PBS driver would have had to fail
> probing if set_drvdata never got called.
> > Konrad