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?
[...]
> +
> +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
> + /* 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?
> + }
> +
> + 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?
> +}
> +
> +/**
> + * 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.
> + 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.
[...]
> +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
