From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, vkoul@kernel.org Cc: yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH 2/3] soundwire: qcom: add auto enumeration support Date: Tue, 2 Mar 2021 10:33:49 +0000 [thread overview] Message-ID: <4721dd27-c8ce-f988-3c10-794841390656@linaro.org> (raw) In-Reply-To: <0c551b23-7ed4-59d7-72c2-284bdf8584f1@linux.intel.com> On 26/02/2021 17:44, Pierre-Louis Bossart wrote: > >> +static int qcom_swrm_enumerate(struct sdw_bus *bus) >> +{ >> + struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); >> + struct sdw_slave *slave, *_s; >> + struct sdw_slave_id id; >> + u32 val1, val2; >> + u64 addr; >> + int i; >> + char *buf1 = (char *)&val1, *buf2 = (char *)&val2; >> + >> + for (i = 1; i < (SDW_MAX_DEVICES + 1); i++) { > > I don't understand the (SDW_MAX_DEVICES + 1)? If you mean +1 then probably we can add <= instead of just < to make it look like other parts of code in bus.c for (i = 1; i <= SDW_MAX_DEVICES; i++) > > >> + /*SCP_Devid5 - Devid 4*/ >> + ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1); >> + >> + /*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/ >> + ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2); > > Do you mind explaining a bit what happens here? > Does the hardware issue commands to read all DevID registers and set the > device number automagically? exactly the hardware assigns device numbers to slaves automatically, and the devnum can be figured out by the controller driver by reading SWRM_ENUMERATOR_SLAVE_DEV_ID_1/2 registers! > If yes, then in SoundWire parlance the enumeration is complete. What you > are doing below is no longer part of the enumeration. yes, enumeration is complete by the hardware, however the controller driver need to know the dev number assigned by the hardware, this routine is doing that! > > >> + >> + if (!val1 && !val2) >> + break; >> + >> + addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) | >> + ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) | >> + ((u64)buf1[0] << 40); >> + >> + sdw_extract_slave_id(bus, addr, &id); >> + /* Now compare with entries */ >> + list_for_each_entry_safe(slave, _s, &bus->slaves, node) { >> + if (sdw_compare_devid(slave, id) == 0) { >> + u32 status = qcom_swrm_get_n_device_status(ctrl, i); >> + if (status == SDW_SLAVE_ATTACHED) { >> + slave->dev_num = i; >> + mutex_lock(&bus->bus_lock); >> + set_bit(i, bus->assigned); >> + mutex_unlock(&bus->bus_lock); >> + >> + } > > And that part is strange as well. The bus->assigned bit should be set > even if the Slave is not in the list provided by platform firmware. It's > really tracking the state of the hardware, and it should not be > influenced by what software knows to manage. Am not 100% sure If I understand the concern here, but In normal (non auto enum) cases this bit is set by the bus code while its doing enumeration to assign a dev number from the assigned bitmap! However in this case where auto enumeration happens it makes sense to set this here with matching dev number! AFAIU from code, each bit in this bitmap corresponds to slave dev number! > >> + break; >> + } >> + } >> + } >> + >> + complete(&ctrl->enumeration); > > you have init_completion() and complete() in this patch, but no > wait_for_completion(), so that should be added in a later patch, no? make sense, will move that to other patch! --srini > >> + return 0; >> +} >> + >> static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) >> { >> struct qcom_swrm_ctrl *swrm = dev_id; >> - u32 value, intr_sts, intr_sts_masked; >> + u32 value, intr_sts, intr_sts_masked, slave_status; >> u32 i; >> u8 devnum = 0; >> int ret = IRQ_HANDLED; >> @@ -382,10 +443,19 @@ static irqreturn_t qcom_swrm_irq_handler(int >> irq, void *dev_id) >> break; >> case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED: >> case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS: >> - dev_err_ratelimited(swrm->dev, "%s: SWR new slave >> attached\n", >> + dev_err_ratelimited(swrm->dev, "%s: SWR slave status >> changed\n", >> __func__); >> - qcom_swrm_get_device_status(swrm); >> - sdw_handle_slave_status(&swrm->bus, swrm->status); >> + swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, >> &slave_status); >> + if (swrm->slave_status == slave_status) { >> + dev_err(swrm->dev, "Slave status not changed %x\n", >> + slave_status); >> + break; >> + } else { >> + dev_err(swrm->dev, "Slave status handle %x\n", >> slave_status); >> + qcom_swrm_get_device_status(swrm); >> + qcom_swrm_enumerate(&swrm->bus); >> + sdw_handle_slave_status(&swrm->bus, swrm->status); >> + } >> break; >> case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET: >> dev_err_ratelimited(swrm->dev, >> @@ -472,8 +542,8 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl >> *ctrl) >> ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val); >> - /* Disable Auto enumeration */ >> - ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0); >> + /* Enable Auto enumeration */ >> + ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1); >> ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK; >> /* Mask soundwire interrupts */ >> @@ -507,6 +577,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl >> *ctrl) >> ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, >> SWRM_INTERRUPT_STATUS_RMSK); >> } >> + ctrl->slave_status = 0; >> return 0; >> } >> @@ -1068,6 +1139,7 @@ static int qcom_swrm_probe(struct >> platform_device *pdev) >> dev_set_drvdata(&pdev->dev, ctrl); >> mutex_init(&ctrl->port_lock); >> init_completion(&ctrl->broadcast); >> + init_completion(&ctrl->enumeration); >> ctrl->bus.ops = &qcom_swrm_ops; >> ctrl->bus.port_ops = &qcom_swrm_port_ops; >>
WARNING: multiple messages have this Message-ID (diff)
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>, vkoul@kernel.org Cc: sanyog.r.kale@intel.com, yung-chuan.liao@linux.intel.com, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH 2/3] soundwire: qcom: add auto enumeration support Date: Tue, 2 Mar 2021 10:33:49 +0000 [thread overview] Message-ID: <4721dd27-c8ce-f988-3c10-794841390656@linaro.org> (raw) In-Reply-To: <0c551b23-7ed4-59d7-72c2-284bdf8584f1@linux.intel.com> On 26/02/2021 17:44, Pierre-Louis Bossart wrote: > >> +static int qcom_swrm_enumerate(struct sdw_bus *bus) >> +{ >> + struct qcom_swrm_ctrl *ctrl = to_qcom_sdw(bus); >> + struct sdw_slave *slave, *_s; >> + struct sdw_slave_id id; >> + u32 val1, val2; >> + u64 addr; >> + int i; >> + char *buf1 = (char *)&val1, *buf2 = (char *)&val2; >> + >> + for (i = 1; i < (SDW_MAX_DEVICES + 1); i++) { > > I don't understand the (SDW_MAX_DEVICES + 1)? If you mean +1 then probably we can add <= instead of just < to make it look like other parts of code in bus.c for (i = 1; i <= SDW_MAX_DEVICES; i++) > > >> + /*SCP_Devid5 - Devid 4*/ >> + ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_1(i), &val1); >> + >> + /*SCP_Devid3 - DevId 2 Devid 1 Devid 0*/ >> + ctrl->reg_read(ctrl, SWRM_ENUMERATOR_SLAVE_DEV_ID_2(i), &val2); > > Do you mind explaining a bit what happens here? > Does the hardware issue commands to read all DevID registers and set the > device number automagically? exactly the hardware assigns device numbers to slaves automatically, and the devnum can be figured out by the controller driver by reading SWRM_ENUMERATOR_SLAVE_DEV_ID_1/2 registers! > If yes, then in SoundWire parlance the enumeration is complete. What you > are doing below is no longer part of the enumeration. yes, enumeration is complete by the hardware, however the controller driver need to know the dev number assigned by the hardware, this routine is doing that! > > >> + >> + if (!val1 && !val2) >> + break; >> + >> + addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) | >> + ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) | >> + ((u64)buf1[0] << 40); >> + >> + sdw_extract_slave_id(bus, addr, &id); >> + /* Now compare with entries */ >> + list_for_each_entry_safe(slave, _s, &bus->slaves, node) { >> + if (sdw_compare_devid(slave, id) == 0) { >> + u32 status = qcom_swrm_get_n_device_status(ctrl, i); >> + if (status == SDW_SLAVE_ATTACHED) { >> + slave->dev_num = i; >> + mutex_lock(&bus->bus_lock); >> + set_bit(i, bus->assigned); >> + mutex_unlock(&bus->bus_lock); >> + >> + } > > And that part is strange as well. The bus->assigned bit should be set > even if the Slave is not in the list provided by platform firmware. It's > really tracking the state of the hardware, and it should not be > influenced by what software knows to manage. Am not 100% sure If I understand the concern here, but In normal (non auto enum) cases this bit is set by the bus code while its doing enumeration to assign a dev number from the assigned bitmap! However in this case where auto enumeration happens it makes sense to set this here with matching dev number! AFAIU from code, each bit in this bitmap corresponds to slave dev number! > >> + break; >> + } >> + } >> + } >> + >> + complete(&ctrl->enumeration); > > you have init_completion() and complete() in this patch, but no > wait_for_completion(), so that should be added in a later patch, no? make sense, will move that to other patch! --srini > >> + return 0; >> +} >> + >> static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) >> { >> struct qcom_swrm_ctrl *swrm = dev_id; >> - u32 value, intr_sts, intr_sts_masked; >> + u32 value, intr_sts, intr_sts_masked, slave_status; >> u32 i; >> u8 devnum = 0; >> int ret = IRQ_HANDLED; >> @@ -382,10 +443,19 @@ static irqreturn_t qcom_swrm_irq_handler(int >> irq, void *dev_id) >> break; >> case SWRM_INTERRUPT_STATUS_NEW_SLAVE_ATTACHED: >> case SWRM_INTERRUPT_STATUS_CHANGE_ENUM_SLAVE_STATUS: >> - dev_err_ratelimited(swrm->dev, "%s: SWR new slave >> attached\n", >> + dev_err_ratelimited(swrm->dev, "%s: SWR slave status >> changed\n", >> __func__); >> - qcom_swrm_get_device_status(swrm); >> - sdw_handle_slave_status(&swrm->bus, swrm->status); >> + swrm->reg_read(swrm, SWRM_MCP_SLV_STATUS, >> &slave_status); >> + if (swrm->slave_status == slave_status) { >> + dev_err(swrm->dev, "Slave status not changed %x\n", >> + slave_status); >> + break; >> + } else { >> + dev_err(swrm->dev, "Slave status handle %x\n", >> slave_status); >> + qcom_swrm_get_device_status(swrm); >> + qcom_swrm_enumerate(&swrm->bus); >> + sdw_handle_slave_status(&swrm->bus, swrm->status); >> + } >> break; >> case SWRM_INTERRUPT_STATUS_MASTER_CLASH_DET: >> dev_err_ratelimited(swrm->dev, >> @@ -472,8 +542,8 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl >> *ctrl) >> ctrl->reg_write(ctrl, SWRM_MCP_FRAME_CTRL_BANK_ADDR(0), val); >> - /* Disable Auto enumeration */ >> - ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 0); >> + /* Enable Auto enumeration */ >> + ctrl->reg_write(ctrl, SWRM_ENUMERATOR_CFG_ADDR, 1); >> ctrl->intr_mask = SWRM_INTERRUPT_STATUS_RMSK; >> /* Mask soundwire interrupts */ >> @@ -507,6 +577,7 @@ static int qcom_swrm_init(struct qcom_swrm_ctrl >> *ctrl) >> ctrl->reg_write(ctrl, SWRM_INTERRUPT_CPU_EN, >> SWRM_INTERRUPT_STATUS_RMSK); >> } >> + ctrl->slave_status = 0; >> return 0; >> } >> @@ -1068,6 +1139,7 @@ static int qcom_swrm_probe(struct >> platform_device *pdev) >> dev_set_drvdata(&pdev->dev, ctrl); >> mutex_init(&ctrl->port_lock); >> init_completion(&ctrl->broadcast); >> + init_completion(&ctrl->enumeration); >> ctrl->bus.ops = &qcom_swrm_ops; >> ctrl->bus.port_ops = &qcom_swrm_port_ops; >>
next prev parent reply other threads:[~2021-03-02 11:02 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-26 17:02 [PATCH 0/3] soundwire: qcom: add Clock Stop and Auto Enumeration support Srinivas Kandagatla 2021-02-26 17:02 ` Srinivas Kandagatla 2021-02-26 17:02 ` [PATCH 1/3] soundwire: export sdw_compare_devid() and sdw_extract_slave_id() Srinivas Kandagatla 2021-02-26 17:02 ` Srinivas Kandagatla 2021-02-26 17:02 ` [PATCH 2/3] soundwire: qcom: add auto enumeration support Srinivas Kandagatla 2021-02-26 17:02 ` Srinivas Kandagatla 2021-02-26 17:44 ` Pierre-Louis Bossart 2021-02-26 17:44 ` Pierre-Louis Bossart 2021-03-02 10:33 ` Srinivas Kandagatla [this message] 2021-03-02 10:33 ` Srinivas Kandagatla 2021-03-02 14:34 ` Pierre-Louis Bossart 2021-03-02 14:34 ` Pierre-Louis Bossart 2021-03-03 9:38 ` Srinivas Kandagatla 2021-03-03 9:38 ` Srinivas Kandagatla 2021-03-03 16:35 ` Pierre-Louis Bossart 2021-03-05 10:39 ` Srinivas Kandagatla 2021-03-05 16:19 ` Pierre-Louis Bossart 2021-03-05 16:57 ` Srinivas Kandagatla 2021-02-26 17:02 ` [PATCH 3/3] soundwire: qcom: add clock stop via runtime pm support Srinivas Kandagatla 2021-02-26 17:02 ` Srinivas Kandagatla 2021-02-26 17:41 ` Pierre-Louis Bossart 2021-02-26 17:41 ` Pierre-Louis Bossart 2021-03-03 11:46 ` Srinivas Kandagatla 2021-03-03 11:46 ` Srinivas Kandagatla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4721dd27-c8ce-f988-3c10-794841390656@linaro.org \ --to=srinivas.kandagatla@linaro.org \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pierre-louis.bossart@linux.intel.com \ --cc=sanyog.r.kale@intel.com \ --cc=vkoul@kernel.org \ --cc=yung-chuan.liao@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.