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
