by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH V3 2/6] of/slimbus: OF helper for SLIMbus

On Mon, Aug 03, 2015 at 11:13:55AM -0500, Rob Herring wrote:
> On Mon, Aug 3, 2015 at 1:59 AM, Sagar Dharia <[email protected]> wrote:

> > +Optional:
> > +- compatible - Slave devices can use compatible field to have a name.
> > + If this field is missing, name of the device will be
> > + determined using slave's enumeration address.
> > + (e.g. in the example below, slave's name will be:
> > + 0x217:0x60:0x1:0x0)

> Are devices discoverable and uniquely identifiable? This would be
> something like a VID/PID which can be read in a generic way. It looks
> like the address contains this info, but can you discover the
> addresses of devices on the bus? If not compatible should not be
> optional.

Yes, the bus is enumerable. However in essentially all implementations
devices won't enumerate without some additional non-standard work being
done to power them on since applications tend to be extremely power
sensitive relative to the basic operating cost of the bus. Which is
entertaining.

This probably means that the binding needs to say that the compatible is
optional but that for all practical purposes Linux requires it until
such time as someone decides it's worth supporting systems that can
usefully enumerate.

Attachments:

(No filename) (1.26 kB)
signature.asc (473.00 B)
Digital signature Download all attachments

2015-08-06 19:11:03

by Sagar Dharia

Hi Mark,

On 8/14/2015 12:39 PM, Mark Brown wrote:
> On Mon, Aug 03, 2015 at 12:59:49AM -0600, Sagar Dharia wrote:
>
>> @@ -459,6 +459,8 @@ int slim_register_controller(struct slim_controller *ctrl)
>> mutex_init(&ctrl->m_ctrl);
>> spin_lock_init(&ctrl->tx.lock);
>> spin_lock_init(&ctrl->rx.lock);
>> + mutex_init(&ctrl->sched.m_reconf);
>> + init_completion(&ctrl->sched.pause_comp);
> Should there not be more interaction with the rest of the framework on
> clock pauses - the bus will need to be started to do transfers for
> example?

I'm relying on controller's pm_runtime as you pointed out for
entering/exiting clock-pause.
The reason to initiate clock-pause from controller was more on the lines
of clock-pause sequence being expensive, and tight coupling between
controller/framer-clocks and clock-pause.
Three reconfiguration-transfer-commands have to be broadcast on the
lines to enter clock-pause, and exiting it is needs 'wakeup' callback
for controller.
Doing this per transfer will be a big overhead.
Using pm_runtime's autosuspend feature allows the controller-device to
enter clock-pause after periods of inactivity to avoid thrashing.
As a trade-off, I am proposing that controller should initiate
clock-pause from its pm_runtime callbacks, and framework should provide
the means to do it.
Also, typically clocks of the controller are coupled with clock-pause
and clocks cannot be turned off until the clock-pause sequence is sent
on the bus since framer has to keep clocking the bus.

Thanks
Sagar

>
>> +#include <linux/slimbus.h>
>> +/**
> Missing blank line.
>
>> + * slim_ctrl_clk_pause: Called by slimbus controller to enter/exit 'clock pause'
> Is the controller the best place to initiate bus pausing? It's
> surprising to me that it would be, the bus being idle isn't something
> that needs controller specific knowledge so I'd expect the framework to
> have standard support for this.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

2015-08-17 23:44:26

by Sagar Dharia

[permalink] [raw]

Subject: Re: [PATCH V3 4/6] slim: qcom: Add Qualcomm Slimbus controller driver

On 8/14/2015 12:31 PM, Mark Brown wrote:
> On Mon, Aug 03, 2015 at 12:59:48AM -0600, Sagar Dharia wrote:
>
>> +/**
>> + * Qualcomm slimbus controller may experience interference on the lines
>> + * causing some early messages (e.g. logical-address assignment) to be
>> + * NACKed. Retry after sometime (typically 1 superframe)
>> + */
>> +static void msm_slim_wait_retry(struct msm_slim_ctrl *dev)
>> +{
>> + int msec_per_frm = 0;
>> + int sfr_per_sec;
>> +
>> + /* Wait for 1 superframe, or default time and then retry */
>> + sfr_per_sec = dev->framer.superfreq /
>> + (1 << (SLIM_MAX_CLK_GEAR - dev->ctrl.clkgear));
>> + if (sfr_per_sec)
>> + msec_per_frm = MSEC_PER_SEC / sfr_per_sec;
>> + if (msec_per_frm < DEF_RETRY_MS)
>> + msec_per_frm = DEF_RETRY_MS;
>> + msleep(msec_per_frm);
>> +}
> This looks like logic which might be usefully pulled out to the
> framework - I bet this isn't an issue unique to your controller, the
> users (well, one user) look like cases where the bus might be
> destabilised by devices powering up and trying to enumerate under less
> than ideal electrical conditions which seems like something that's
> likely to affect other hardware.

I agree, I will move this to the framework.

Thanks
Sagar

>
>> + rclk = devm_clk_get(&pdev->dev, "core_clk");
>> + if (IS_ERR(rclk)) {
>> + /* unlikely that this is probe-defer */
>> + dev_err(&pdev->dev, "rclk get failed:%ld\n", PTR_ERR(rclk));
>> + devm_clk_put(&pdev->dev, hclk);
> No need to call devm_clk_put() explicitly in your probe function.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation