| Message ID | 20210226170250.9067-3-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: qcom: add Clock Stop and Auto Enumeration support | expand |
> +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)? > + /*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? If yes, then in SoundWire parlance the enumeration is complete. What you are doing below is no longer part of the enumeration. > + > + 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. > + 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? > + 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; >
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; >>
>>> + 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! Yes, but the point was "why do you compare with information coming from platform firmware"? if the hardware reports the presence of devices on the link, why not use the information as is? You recently added code that helps us deal with devices that are not listed in DT or ACPI tables, so why would we filter in this specific loop? >>> + 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! Actually on this one comment that I missed last time is that you are using a completion only for the resume() case, and I think it should also be used for the regular probe() case, no?
On 02/03/2021 14:34, Pierre-Louis Bossart wrote: > > >>>> + 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! > > Yes, but the point was "why do you compare with information coming from > platform firmware"? if the hardware reports the presence of devices on This is the logic that hardware IP document suggests to use to get get the correct the device number associated with the slave! > the link, why not use the information as is? > > You recently added code that helps us deal with devices that are not > listed in DT or ACPI tables, so why would we filter in this specific loop? > >>>> + 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! > > Actually on this one comment that I missed last time is that you are > using a completion only for the resume() case, and I think it should > also be used for the regular probe() case, no? Good Idea, I can try that and see how to works out! --srini >
On 3/3/21 3:38 AM, Srinivas Kandagatla wrote: > > > On 02/03/2021 14:34, Pierre-Louis Bossart wrote: >> >> >>>>> + 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! >> >> Yes, but the point was "why do you compare with information coming >> from platform firmware"? if the hardware reports the presence of >> devices on > > This is the logic that hardware IP document suggests to use to get get > the correct the device number associated with the slave! > > >> the link, why not use the information as is? >> >> You recently added code that helps us deal with devices that are not >> listed in DT or ACPI tables, so why would we filter in this specific >> loop? I don't think my point was understood, so let me try to explain it differently. it's my understanding that the hardware reads the DevID registers and writes a Device Number - so that the entire enumeration sequence started by reading DevID0 and finished by a successful write to SCP_DevNum is handled in hardware. The question is: what happens if that device is NOT described in the Device Tree data? The loop over bus->slaves will not find this device by comparing with known devID values, so the set_bit(i, bus->assigned) will not happen.
On 03/03/2021 16:35, Pierre-Louis Bossart wrote: > > > On 3/3/21 3:38 AM, Srinivas Kandagatla wrote: >> >> >> On 02/03/2021 14:34, Pierre-Louis Bossart wrote: >>> >>> >>>>>> + 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! >>> >>> Yes, but the point was "why do you compare with information coming >>> from platform firmware"? if the hardware reports the presence of >>> devices on >> >> This is the logic that hardware IP document suggests to use to get get >> the correct the device number associated with the slave! >> >> >>> the link, why not use the information as is? >>> >>> You recently added code that helps us deal with devices that are not >>> listed in DT or ACPI tables, so why would we filter in this specific >>> loop? > > I don't think my point was understood, so let me try to explain it > differently. > > it's my understanding that the hardware reads the DevID registers and > writes a Device Number - so that the entire enumeration sequence started > by reading DevID0 and finished by a successful write to SCP_DevNum is > handled in hardware. > > The question is: what happens if that device is NOT described in the > Device Tree data? The loop over bus->slaves will not find this device by > comparing with known devID values, so the set_bit(i, bus->assigned) will > not happen. yes, that is true, There is no way we can assign a dev_number to the device which is not enumerated on the bus! Am sure this is the same behavior with soundwire core too, atleast form the code I can see it sets the assigned bit for only the devices that are enumerated on the bus! Not all the devices specified in DT! Unless I missed something! --srini --srini > >
>>>>>>> + 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! >>>> >>>> Yes, but the point was "why do you compare with information coming >>>> from platform firmware"? if the hardware reports the presence of >>>> devices on >>> >>> This is the logic that hardware IP document suggests to use to get >>> get the correct the device number associated with the slave! >>> >>> >>>> the link, why not use the information as is? >>>> >>>> You recently added code that helps us deal with devices that are not >>>> listed in DT or ACPI tables, so why would we filter in this specific >>>> loop? >> >> I don't think my point was understood, so let me try to explain it >> differently. >> >> it's my understanding that the hardware reads the DevID registers and >> writes a Device Number - so that the entire enumeration sequence >> started by reading DevID0 and finished by a successful write to >> SCP_DevNum is handled in hardware. >> >> The question is: what happens if that device is NOT described in the >> Device Tree data? The loop over bus->slaves will not find this device >> by comparing with known devID values, so the set_bit(i, bus->assigned) >> will not happen. > > yes, that is true, There is no way we can assign a dev_number to the > device which is not enumerated on the bus! > > Am sure this is the same behavior with soundwire core too, atleast form > the code I can see it sets the assigned bit for only the devices that > are enumerated on the bus! Not all the devices specified in DT! > Unless I missed something! I am talking about the other way around, where a device is present and enumerated on the bus but not listed in DT. In that case the hardware did assign a device number but bus->assigned will not be set.
On 05/03/2021 16:19, Pierre-Louis Bossart wrote: >>> >>> >>> The question is: what happens if that device is NOT described in the >>> Device Tree data? The loop over bus->slaves will not find this device >>> by comparing with known devID values, so the set_bit(i, >>> bus->assigned) will not happen. >> >> yes, that is true, There is no way we can assign a dev_number to the >> device which is not enumerated on the bus! >> >> Am sure this is the same behavior with soundwire core too, atleast >> form the code I can see it sets the assigned bit for only the devices >> that are enumerated on the bus! Not all the devices specified in DT! >> Unless I missed something! > > I am talking about the other way around, where a device is present and > enumerated on the bus but not listed in DT. In that case the hardware > did assign a device number but bus->assigned will not be set. thanks for your patience! Ah, I understand it now!, yes that part is missing! adding Something like what core does in qcom driver should fix it! if (!found) { sdw_slave_add(bus, &id, NULL); dev_err(bus->dev, "Slave Entry not found\n"); } --srini
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index 8438f9812d7c..d90eba6a1e88 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -57,6 +57,8 @@ #define SWRM_CMD_FIFO_RD_FIFO_ADDR 0x318 #define SWRM_RD_FIFO_CMD_ID_MASK GENMASK(11, 8) #define SWRM_ENUMERATOR_CFG_ADDR 0x500 +#define SWRM_ENUMERATOR_SLAVE_DEV_ID_1(m) (0x530 + 0x8 * (m)) +#define SWRM_ENUMERATOR_SLAVE_DEV_ID_2(m) (0x534 + 0x8 * (m)) #define SWRM_MCP_FRAME_CTRL_BANK_ADDR(m) (0x101C + 0x40 * (m)) #define SWRM_MCP_FRAME_CTRL_BANK_COL_CTRL_BMSK GENMASK(2, 0) #define SWRM_MCP_FRAME_CTRL_BANK_ROW_CTRL_BMSK GENMASK(7, 3) @@ -121,6 +123,7 @@ struct qcom_swrm_ctrl { struct regmap *regmap; void __iomem *mmio; struct completion broadcast; + struct completion enumeration; struct work_struct slave_work; /* Port alloc/free lock */ struct mutex port_lock; @@ -143,6 +146,7 @@ struct qcom_swrm_ctrl { enum sdw_slave_status status[SDW_MAX_DEVICES]; int (*reg_read)(struct qcom_swrm_ctrl *ctrl, int reg, u32 *val); int (*reg_write)(struct qcom_swrm_ctrl *ctrl, int reg, int val); + u32 slave_status; }; struct qcom_swrm_data { @@ -342,6 +346,7 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl) int i; ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val); + ctrl->slave_status = val; for (i = 0; i < SDW_MAX_DEVICES; i++) { u32 s; @@ -352,10 +357,66 @@ static void qcom_swrm_get_device_status(struct qcom_swrm_ctrl *ctrl) } } +static int qcom_swrm_get_n_device_status(struct qcom_swrm_ctrl *ctrl, int devnum) +{ + u32 val; + + ctrl->reg_read(ctrl, SWRM_MCP_SLV_STATUS, &val); + val = (val >> (devnum * SWRM_MCP_SLV_STATUS_SZ)); + val &= SWRM_MCP_SLV_STATUS_MASK; + + return val; +} + +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++) { + /*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); + + 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); + + } + break; + } + } + } + + complete(&ctrl->enumeration); + 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;
Qualcomm SoundWire controller supports Auto Enumeration of the devices within the IP. This patch enables support for this feature. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/qcom.c | 84 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 78 insertions(+), 6 deletions(-)