Message-ID: <e79dae39-faec-b2a4-82e6-e11325904f3e@linux.intel.com>
Date:   Tue, 14 Feb 2023 07:21:33 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.1
Subject: Re: [PATCH V2 2/8] soundwire: amd: Add support for AMD Manager driver
Content-Language: en-US
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>, vkoul@kernel.org
Cc:     amadeuszx.slawinski@linux.intel.com, Mario.Limonciello@amd.com,
        Sunil-kumar.Dommati@amd.com, Basavaraj.Hiregoudar@amd.com,
        Mastan.Katragadda@amd.com, Arungopal.kondaveeti@amd.com,
        Bard Liao <yung-chuan.liao@linux.intel.com>,
        Sanyog Kale <sanyog.r.kale@intel.com>,
        open list <linux-kernel@vger.kernel.org>,
        "moderated list:SOUNDWIRE SUBSYSTEM" <alsa-devel@alsa-project.org>
References: <20230213094031.2231058-1-Vijendar.Mukunda@amd.com>
 <20230213094031.2231058-3-Vijendar.Mukunda@amd.com>
 <a3a75ead-5430-ae32-a6ae-78314bc637f1@linux.intel.com>
 <7b5198f5-1894-5ab5-f84b-410cf102268d@amd.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <7b5198f5-1894-5ab5-f84b-410cf102268d@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


>>> +static void amd_sdw_probe_work(struct work_struct *work)
>>> +{
>>> +	struct amd_sdw_manager *amd_manager = container_of(work, struct amd_sdw_manager,
>>> +							   probe_work);
>>> +	struct sdw_master_prop *prop;
>>> +	int ret;
>>> +
>>> +	prop = &amd_manager->bus.prop;
>>> +	if (!prop->hw_disabled) {
>>> +		amd_enable_sdw_pads(amd_manager);
>>> +		ret = amd_init_sdw_manager(amd_manager);
>>> +		if (ret)
>>> +			return;
>>> +		amd_enable_sdw_interrupts(amd_manager);
>>> +		ret = amd_enable_sdw_manager(amd_manager);
>>> +		if (ret)
>>> +			return;
>>> +		amd_sdw_set_frameshape(amd_manager);
>>> +	}
>>> +}
>> There should be an explanation as to why you need a workqueue to
>> complete the probe.
> We want to separate the manager probe sequence and start up sequence.
> we will add the comment.

Do you need to split in two? For Intel, on some platforms we had a clear
power dependency, we had to wait until parts of the DSP were powered
before accessing SHIM registers, so we called the startup() when those
dependencies were resolved.

I am not sure you can count on the probe_work to enforce any kind of
delay, worst case the work function could be scheduled immediately.