Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 16.04.2021 19:31, Mark Brown wrote: > On Fri, Apr 16, 2021 at 04:03:05PM +, codrin.ciubota...@microchip.com > wrote: > >> Thank you for the links! So basically the machine driver disappears and >> all the components will be visible in user-space. > > Not entirely - you still need something to say how they're wired > together but it'll be a *lot* simpler for anything that currently used > DPCM. Right, device-tree/acpi wiring is not enough... > >> If there is a list with the 'steps' or tasks to achieve this? I can try >> to pitch in. > > Not really written down that I can think of. I think the next steps > that I can think of right now are unfortunately bigger and harder ones, > mainly working out a way to represent digital configuration as a graph > that can be attached to/run in parallel with DAPM other people might > have some better ideas though. Sorry, I appreciate that this isn't > super helpful :/ > I think I have good picture, a more robust card->dai_link, not with just FEs and BEs ... I will try to monitor the alsa list more, see what other people are working on. Thank you for your time! Best regards, Codrin
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 15.04.2021 20:25, Mark Brown wrote: > On Thu, Apr 15, 2021 at 04:56:00PM +, codrin.ciubota...@microchip.com > wrote: > >> Are there any plans for refactoring DPCM? any ideas ongoing? I also have >> some changes for PCM dmaengine, in the same 'style', similar to what I >> sent some time ago... >> I can adjust to different ideas, if there are any, but, for a start, can >> anyone confirm that the problem I am trying to fix is real? > > Lars-Peter's presentation from ELC in 2016 (!) is probably the clearest > summary of the ideas: > > https://elinux.org/images/e/e7/Audio_on_Linux.pdf > https://youtu.be/6oQF2TzCYtQ > > Essentially the idea is to represent everything, including the DSPs, as > ASoC components and be able to represent the digital links between > components in the DAPM graph in the same way we currently represent > analogue links. This means we need a way to propagate digital > configuration along the DAPM graph (or a parallel, cross linked digital > one). Sadly I'm not really aware of anyone actively working on the > actual conversion at the minute, Morimoto-san has done a lot of great > preparatory work to make everything a component which makes the whole > thing more tractable. > Thank you for the links! So basically the machine driver disappears and all the components will be visible in user-space. If there is a list with the 'steps' or tasks to achieve this? I can try to pitch in. Best regards, Codrin
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 15.04.2021 19:17, Mark Brown wrote: > On Wed, Apr 14, 2021 at 02:58:10PM +, codrin.ciubota...@microchip.com > wrote: > >> How about using a different API for ASoC only, since that's the place of >> DPCM. Only drivers that do not involve DSPs would have to to be changed >> to call the new snd_pcm_hw_rule_add() variant. >> Another solution would be to have a different snd_soc_pcm_runtime for BE >> interfaces (with a new hw_constraints member of course). What do you think? > > I'm really not convinced we want to continue to pile stuff on top of > DPCM, it is just fundamentally not up to modelling what modern systems > are able to do - it's already making things more fragile than they > should be and more special cases seems like it's going to end up making > that worse. That said I've not seen the code but... > Are there any plans for refactoring DPCM? any ideas ongoing? I also have some changes for PCM dmaengine, in the same 'style', similar to what I sent some time ago... I can adjust to different ideas, if there are any, but, for a start, can anyone confirm that the problem I am trying to fix is real? Best regards, Codrin
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 23.03.2021 16:18, codrin.ciubota...@microchip.com wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On 23.03.2021 14:15, Jaroslav Kysela wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >> content is safe >> >> Dne 23. 03. 21 v 12:43 Codrin Ciubotariu napsal(a): >> >>> To achieve this, the first thing needed is to detect whether a HW >>> constraint rule is enforced by a FE or a BE DAI. This means that >>> snd_pcm_hw_rule_add() needs to be able to differentiate between the two >>> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is >>> replaced with a pointer to struct snd_pcm_substream, to be able to reach >>> substream->pcm->internal to differentiate between FE and BE DAIs. >> >> Think about other not-so-invasive solution. What about to use >> 'runtime->private_data' (struct snd_soc_pcm_runtime *) to determine FE / BE? >> > > I think struct snd_soc_pcm_runtime * is placed in > substream->private_data, while runtime->private_data is used more by the > platform drivers. runtime->trigger_master could be an idea, but it looks > like it's initialized just before the trigger callback is called, way > after the constraint rules are added and I am not sure it can be > initialized earlier... > > Best regards, > Codrin > How about using a different API for ASoC only, since that's the place of DPCM. Only drivers that do not involve DSPs would have to to be changed to call the new snd_pcm_hw_rule_add() variant. Another solution would be to have a different snd_soc_pcm_runtime for BE interfaces (with a new hw_constraints member of course). What do you think? Thanks! Codrin
Re: [PATCH 02/17] ASoC: atmel: fix shadowed variable
On 26.03.2021 23:59, Pierre-Louis Bossart wrote:
> Fix cppcheck warning:
>
> sound/soc/atmel/atmel-classd.c:51:14: style: Local variable 'pwm_type'
> shadows outer variable [shadowVariable]
> const char *pwm_type;
> ^
> sound/soc/atmel/atmel-classd.c:226:27: note: Shadowed declaration
> static const char * const pwm_type[] = {
>^
> sound/soc/atmel/atmel-classd.c:51:14: note: Shadow variable
> const char *pwm_type;
> ^
>
> Signed-off-by: Pierre-Louis Bossart
Reviewed-by: Codrin Ciubotariu
Thanks!
Re: [PATCH 03/17] ASoC: atmel: atmel-i2s: remove useless initialization
On 26.03.2021 23:59, Pierre-Louis Bossart wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Cppcheck complains: > > sound/soc/atmel/atmel-i2s.c:628:6: style: Redundant initialization for 'err'. > The initialized value is overwritten before it is read. > [redundantInitialization] > err = devm_request_irq(&pdev->dev, irq, atmel_i2s_interrupt, 0, > ^ > sound/soc/atmel/atmel-i2s.c:598:10: note: err is initialized > int err = -ENXIO; > ^ > sound/soc/atmel/atmel-i2s.c:628:6: note: err is overwritten > err = devm_request_irq(&pdev->dev, irq, atmel_i2s_interrupt, 0, > ^ > > Signed-off-by: Pierre-Louis Bossart Reviewed-by: Codrin Ciubotariu Thanks!
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 24.03.2021 17:28, Pierre-Louis Bossart wrote: >> I am using hw_params_fixup, but it's not enough. The first thing I do is >> to not add the BE HW constraint rules in runtime->hw_constraints, >> because this will potentially affect the FE HW params. If the FE does >> sampling rate conversion, for example, applying the sampling rate >> constrain rules from a BE codec on FE is useless. The second thing I do >> is to apply these BE HW constraint rules to the BE HW params. It's true >> that the BE HW params can be fine tuned via hw_params_fixup (topology, >> device-tree or even static parameters) and set in such a way that avoid >> the BE HW constraints, so we could ignore the constraint rules added by >> their drivers. It's not every elegant and running the BE HW constraint >> rules for the fixup BE HW params can be a sanity check. Also, I am >> thinking that if the FE does no conversion (be_hw_params_fixup missing) >> and more than one BEs are available, applying the HW constraint rules on >> the same set of BE HW params could rule out the incompatible BE DAI >> links and start the compatible ones only. I am not sure this is a real >> usecase. > > Even after a second cup of coffee I was not able to follow why the > hw_params_fixup was not enough? That paragraph is rather dense. Right, sorry about that. :) > > And to be frank I don't fully understand the problem statement above: > "separate the FE HW constraints from the BE HW constraints". We have > existing solutions with a DSP-based SRC adjusting FE rates to what is > required by the BE dailink. > > Maybe it would help to show examples of what you can do today and what > you cannot, so that we are on the same wavelength on what the > limitations are and how to remove them? For example, ad1934 driver sets a constraint to always have 32 sample bits [1] and this rule will be added in struct snd_pcm_runtime -> hw_constraints [2]. As you can see, this is added early and always. So this rule will affect the HW parameters used from the user-space [3] and, in our example, only audio streams that have formats of 4B containers will be used (S24_LE, S32_LE, etc). For playback, if the audio stream won't have this format, the stream will need to be converted in software, using plug ALSA plugin for example. This is OK for normal PCM: HW params user <--> CPU <---> AD1934 space ^DAI | || -| sample bits constraint rule (32b) For DPCM, we have the same behavior (unfortunately). ad1934, as a BE codec, will add it's rule in the same place, which means that it will again affect the HW parameters set by user-space. So user-space will have to convert all audio streams to have a 32b sample size. If the FE is capable of format conversing, this software conversion is useless. FE HW paramsBE HW params user <--> FE <--> BE CPU <> BE AD1934 space ^DAI DAI| | | --| sample bits constraint rule (32b) The format conversion will be done in software instead of hardware (FE). I hope I was able to be more clear now. Thanks for taking time to understand this. I owe you a coffee. :) Best regards, Codrin [1] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/soc/codecs/ad193x.c#L366 [2] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/core/pcm_lib.c#L1141 [3] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/core/pcm_native.c#L692
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 23.03.2021 21:25, Pierre-Louis Bossart wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
>
> On 3/23/21 6:43 AM, Codrin Ciubotariu wrote:
>> HW constraints are needed to set limitations for HW parameters used to
>> configure the DAIs. All DAIs on the same link must agree upon the HW
>> parameters, so the parameters are affected by the DAIs' features and
>> their limitations. In case of DPCM, the FE DAIs can be used to perform
>> different kind of conversions, such as format or rate changing, bringing
>> the audio stream to a configuration supported by all the DAIs of the BE's
>> link. For this reason, the limitations of the BE DAIs are not always
>> important for the HW parameters between user-space and FE, only for the
>> paratemers between FE and BE DAI links. This brings us to this patch-set,
>> which aims to separate the FE HW constraints from the BE HW constraints.
>> This way, the FE can be used to perform more efficient HW conversions, on
>> the initial audio stream parameters, to parameters supported by the BE
>> DAIs.
>> To achieve this, the first thing needed is to detect whether a HW
>> constraint rule is enforced by a FE or a BE DAI. This means that
>> snd_pcm_hw_rule_add() needs to be able to differentiate between the two
>> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is
>> replaced with a pointer to struct snd_pcm_substream, to be able to reach
>> substream->pcm->internal to differentiate between FE and BE DAIs.
>> This change affects many sound drivers (and one gpu drm driver).
>> All these changes are included in the first patch, to have a better
>> overview of the implications created by this change.
>> The second patch adds a new struct snd_pcm_hw_constraints under struct
>> snd_soc_dpcm_runtime, which is used to store the HW constraint rules
>> added by the BE DAIs. This structure is initialized with a subset of the
>> runtime constraint rules which does not include the rules that affect
>> the buffer or period size. snd_pcm_hw_rule_add() will add the BE rules
>> to the new struct snd_pcm_hw_constraints.
>> The third and last patch will apply the BE rule constraints, after the
>> fixup callback. If the fixup HW parameters do not respect the BE
>> constraint rules, the rules will exit with an error. The FE mask and
>> interval constraints are copied to the BE ones, to satisfy the
>> dai_link->dpcm_merged_* flags. The dai_link->dpcm_merged_* flags are
>> used to know if the FE does format or sampling rate conversion.
>>
>> I tested with ad1934 and wm8731 codecs as BEs, with a not-yet-mainlined
>> ASRC as FE, that can also do format conversion. I realize that the
>> change to snd_pcm_hw_rule_add() has a big impact, even though all the
>> drivers use snd_pcm_hw_rule_add() with substream->runtime, so passing
>> substream instead of runtime is not that risky.
>
> can you use the BE hw_params_fixup instead?
>
> That's what we use for SOF.
>
> The FE hw_params are propagated to the BE, and then the BE can update
> the hw_params based on its own limitations and pass the result
> downstream, e.g. to a codec.
>
> I'll copy below my understanding of the flow, which we discussed
> recently in the SOF team:
>
> my understanding is that we start with the front-end hw_params as the
> basis for the back-end hw_params.
>
> static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params)
> {
> struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
> int ret, stream = substream->stream;
>
> mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
> dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
>
> memcpy(&fe->dpcm[stream].hw_params, params,
> sizeof(struct snd_pcm_hw_params));
> ret = dpcm_be_dai_hw_params(fe, stream);
> <<< the BE is handled first.
> if (ret < 0) {
> dev_err(fe->dev,"ASoC: hw_params BE failed %d\n", ret);
> goto out;
> }
>
> dev_dbg(fe->dev, "ASoC: hw_params FE %s rate %d chan %x fmt %d\n",
> fe->dai_link->name, params_rate(params),
> params_channels(params), params_format(params));
>
> /* call hw_params on the frontend */
> ret = soc_pcm_hw_params(substream, params);
>
> then each BE will be configured
>
> int dpcm_be_dai_hw_params(struct snd_soc_pcm_runtime *fe, int stream)
> {
> struct snd_soc_dpcm *dpcm;
> int ret;
>
> for_each_dpcm_be(fe, stream, dpcm) {
>
> struct snd_soc_pcm_runtime *be = dpcm->be;
> struct snd_pcm_substream *be_substream =
> snd_soc_dpcm_get_substream(be, stream);
>
> /* is this op for this BE ? */
> if (!snd_soc_dpcm_be_can_update(fe, be, stream))
> continue;
>
> /* copy params for each dpcm */
> memcpy(&dpcm->hw_params, &fe->dpcm[stream].hw_params,
> si
Re: [RFC PATCH 0/3] Separate BE DAI HW constraints from FE ones
On 23.03.2021 14:15, Jaroslav Kysela wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > Dne 23. 03. 21 v 12:43 Codrin Ciubotariu napsal(a): > >> To achieve this, the first thing needed is to detect whether a HW >> constraint rule is enforced by a FE or a BE DAI. This means that >> snd_pcm_hw_rule_add() needs to be able to differentiate between the two >> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is >> replaced with a pointer to struct snd_pcm_substream, to be able to reach >> substream->pcm->internal to differentiate between FE and BE DAIs. > > Think about other not-so-invasive solution. What about to use > 'runtime->private_data' (struct snd_soc_pcm_runtime *) to determine FE / BE? > I think struct snd_soc_pcm_runtime * is placed in substream->private_data, while runtime->private_data is used more by the platform drivers. runtime->trigger_master could be an idea, but it looks like it's initialized just before the trigger callback is called, way after the constraint rules are added and I am not sure it can be initialized earlier... Best regards, Codrin
Re: [PATCH 1/7] ASoC: convert Microchip I2SMCC binding to yaml
On 01.03.2021 15:59, Mark Brown wrote: > On Tue, Feb 23, 2021 at 08:19:23PM +0200, Codrin Ciubotariu wrote: >> This patch converts the Microchip I2SMCC bindings to DT schema format >> using json-schema. > > Please place any DT binding conversion patches at the end of patch > serieses, there is a frequent backlog in reviews of those which can > result in everything else getting held up. > I didn't know that, will do so in v2. Thanks!
Re: [PATCH] ASoC: atmel: fix spelling mistake in Kconfig "programable" -> "programmable"
On 16.12.2020 13:26, Colin King wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > From: Colin Ian King > > There are a couple of spelling mistakes in the Kconfig help text. Fix them. > > Signed-off-by: Colin Ian King Reviewed-by: Codrin Ciubotariu Thanks!
Re: [RFC PATCH] ASoC: pcm_dmaengine: Add support for BE DAIs
On 08.12.2020 21:31, Mark Brown wrote: > On Tue, Dec 08, 2020 at 07:26:35PM +, codrin.ciubota...@microchip.com > wrote: > >> I do not know too much about the dummy PCM. It seems like it is creating >> a card without DPCM links and fakes a buffer, which is not quite what I >> need. I will investigate more. > > Right, that's what I was imagining the second runtime you proposed > looking like. > I don't need the whole struct snd_pcm_runtime, only struct snd_pcm_hardware, at least for now. I looked some more and a suitable place would be struct snd_soc_dpcm, since it is created at runtime and used anyway to characterize a FE <-> BE link. What do you think? Also, I noticed that the HW constraints added by a DAI driver (a codec for example) are added to PCM's runtime->hw_constraints, even if the DAI driver is part of a BE. Shouldn't these constraints be applied only to BE and eventually merge them to FE (struct snd_pcm_hardware) if dai_link->dpcm_merged_* are set? Best regards, Codrin
Re: [PATCH v3] i2c: pxa: move to generic GPIO recovery
>> patch not tested. > > LGTM. In case we missed a glitch, we can still revert the patch later. > Sure, just CC me if anything goes wrong.
Re: [RFC PATCH] ASoC: pcm_dmaengine: Add support for BE DAIs
On 08.12.2020 19:04, Mark Brown wrote: > On Wed, Dec 02, 2020 at 10:58:38AM +0200, Codrin Ciubotariu wrote: > >> This patch is more or less incomplete for the described scenario. This >> is because DMAengine's pcm->config is ignored for the BE DAI link, so >> runtime->hw is not updated. Also, since pcm_construct/destruct are not >> called, the DMA channels are allocated only if DT is used. >> Underrun/overrun support would also be a nice to have for the transfers >> involving the buffer allocated for the BE. >> One way to hold trach of these would be to use a substream_be->runtime >> different than the one used for the FE. > >> Please share your thoughts. > > I have a hard time getting enthusiastic about this but I think that's > more DPCM than anything else. Otherwise this looks sensible as far as > it goes. I don't have particular thoughts on exposing errors for the > BEs - we could do a dummy PCM, TBH that bodge was used in the past for > CODEC<->CODEC links but it's obviously inelegant and messy so I'm not > sure it'd help more than just doing something like log the messages in > the kernel. It certainly doesn't seem good to introduce anything that > is visible to userspace but is DPCM specific. > It is DPCM indeed. Well, the first scenario (PCM1) is. I do not intend to create a PCM for the DAI link, when it is a BE. What I meant to say with the runtime->hw is that is mustn't be touched by the BE's platform, but there should be something similar (placed elsewhere) to consider pcm->config. The thing is that, in my case, exactly the same DAI link (same cpu, codec, platform components) can be a normal DAI link or a BE DAI link. I hope to register both PCMs (dynamic with FE and normal), to be able to skip the FE DAI link if it's not needed and use the normal PCM variant, with the same DAI components used in the BE DAI link. For this, I need a platform driver that is not interacting with substream->runtime when is part of a BE DAI link. I don't think anyone else is using the SOC generic dmaengine as a DAI platform component in a BE. I do not know too much about the dummy PCM. It seems like it is creating a card without DPCM links and fakes a buffer, which is not quite what I need. I will investigate more. Thank you for your sharing your ideas! Best regards, Codrin
Re: [PATCH] ASoC: atmel: mchp-spdifrx needs COMMON_CLK
On 04.12.2020 00:38, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Compile-testing this driver on an older platform without CONFIG_COMMON_CLK
> fails with
>
> ERROR: modpost: "clk_set_min_rate" [sound/soc/atmel/snd-soc-mchp-spdifrx.ko]
> undefined!
>
> Make this is a strict dependency.
>
> Fixes: ef265c55c1ac ("ASoC: mchp-spdifrx: add driver for SPDIF RX")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Codrin Ciubotariu
Thanks!
> ---
> sound/soc/atmel/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> index bd8854bfd2ee..142373ec411a 100644
> --- a/sound/soc/atmel/Kconfig
> +++ b/sound/soc/atmel/Kconfig
> @@ -148,6 +148,7 @@ config SND_MCHP_SOC_SPDIFTX
> config SND_MCHP_SOC_SPDIFRX
> tristate "Microchip ASoC driver for boards using S/PDIF RX"
> depends on OF && (ARCH_AT91 || COMPILE_TEST)
> + depends on COMMON_CLK
> select SND_SOC_GENERIC_DMAENGINE_PCM
> select REGMAP_MMIO
> help
> --
> 2.27.0
>
Re: [PATCH] ARM: dts: at91: add serial MFD sub-node for usart
On 02.11.2020 14:55, codrin.ciubota...@microchip.com wrote:
> On 02.11.2020 14:29, Lee Jones wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On Mon, 02 Nov 2020, codrin.ciubota...@microchip.com wrote:
>>
>>> On 02.11.2020 11:01, Lee Jones wrote:
On Fri, 30 Oct 2020, Nicolas Ferre wrote:
> On 30/10/2020 at 12:07, Codrin Ciubotariu wrote:
>> The "atmel,at91sam9260-usart" driver is a MFD driver, so it needs
>> sub-nodes
>> to match the registered platform device. For this reason, we add a serial
>> subnode to all the "atmel,at91sam9260-usart" serial compatible nods. This
>> will also remove the boot warning:
>> "atmel_usart_serial: Failed to locate of_node [id: -2]"
>
> I don't remember this warning was raised previously even if the MFD driver
> was added a while ago (Sept. 2018).
>
> I would say it's due to 466a62d7642f ("mfd: core: Make a best effort
> attempt
> to match devices with the correct of_nodes") which was added on mid August
> and corrected with 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices
> are
> ignored without error") but maybe not covering our case.
>
> So, well, I don't know what's the best option to this change. Moreover, I
> would say that all other USART related properties go into the child not if
> there is a need for one.
>
> Lee, I suspect that we're not the only ones experiencing this ugly warning
> during the boot log: can you point us out how to deal with it for our
> existing atmel_serial.c users?
You should not be instantiating drivers through Device Tree which are
not described there. If the correct representation of the H/W already
exists in Device Tree i.e. no SPI and UART IP really exists, use the
MFD core API to register them utilising the platform API instead.
This should do it:
diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
index 6a8351a4588e2..939bd2332a4f6 100644
--- a/drivers/mfd/at91-usart.c
+++ b/drivers/mfd/at91-usart.c
@@ -17,12 +17,10 @@
static const struct mfd_cell at91_usart_spi_subdev = {
.name = "at91_usart_spi",
- .of_compatible = "microchip,at91sam9g45-usart-spi",
};
static const struct mfd_cell at91_usart_serial_subdev = {
.name = "atmel_usart_serial",
- .of_compatible = "atmel,at91rm9200-usart-serial",
};
static int at91_usart_mode_probe(struct platform_device *pdev)
>>>
>>> [snip]
>>>
>>> Hi Lee, thank you for looking through our usart driver and for sharing
>>> your thoughts. Removing the usage of compatible string means that for
>>> similar serial/SPI IPs we would need to create new platform drivers.
>>
>> Why would you need to do that?
>
> In the case we will have to support another similar IP, but with a
> different set of features. Not a new platform driver from scratch, but
> at least a new struct platform_driver for each variant.
I guess we could use struct mfd_cell.platform_data to select the
features for the serial/SPI. This platform data can be per compatible of
our MFD driver. I will send a patch with the changes you suggested.
Thank you!
Best regards,
Codrin
>
>>
>>> This is not ideal, but it's a solution. What I proposed is more
>>> flexible, but, as you pointed out, I am not sure it correctly describes
>>> the HW, because the decision of whether to use this IP as a serial or a
>>> SPI is a configurable one.
>>>
>>> Thanks and best regards,
>>> Codrin
>>
>> --
>> Lee Jones [李琼斯]
>> Senior Technical Lead - Developer Services
>> Linaro.org │ Open source software for Arm SoCs
>> Follow Linaro: Facebook | Twitter | Blog
>>
>
Re: [PATCH] ARM: dts: at91: add serial MFD sub-node for usart
On 02.11.2020 14:29, Lee Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Mon, 02 Nov 2020, codrin.ciubota...@microchip.com wrote:
>
>> On 02.11.2020 11:01, Lee Jones wrote:
>>> On Fri, 30 Oct 2020, Nicolas Ferre wrote:
>>>
On 30/10/2020 at 12:07, Codrin Ciubotariu wrote:
> The "atmel,at91sam9260-usart" driver is a MFD driver, so it needs
> sub-nodes
> to match the registered platform device. For this reason, we add a serial
> subnode to all the "atmel,at91sam9260-usart" serial compatible nods. This
> will also remove the boot warning:
> "atmel_usart_serial: Failed to locate of_node [id: -2]"
I don't remember this warning was raised previously even if the MFD driver
was added a while ago (Sept. 2018).
I would say it's due to 466a62d7642f ("mfd: core: Make a best effort
attempt
to match devices with the correct of_nodes") which was added on mid August
and corrected with 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices
are
ignored without error") but maybe not covering our case.
So, well, I don't know what's the best option to this change. Moreover, I
would say that all other USART related properties go into the child not if
there is a need for one.
Lee, I suspect that we're not the only ones experiencing this ugly warning
during the boot log: can you point us out how to deal with it for our
existing atmel_serial.c users?
>>>
>>> You should not be instantiating drivers through Device Tree which are
>>> not described there. If the correct representation of the H/W already
>>> exists in Device Tree i.e. no SPI and UART IP really exists, use the
>>> MFD core API to register them utilising the platform API instead.
>>>
>>> This should do it:
>>>
>>> diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
>>> index 6a8351a4588e2..939bd2332a4f6 100644
>>> --- a/drivers/mfd/at91-usart.c
>>> +++ b/drivers/mfd/at91-usart.c
>>> @@ -17,12 +17,10 @@
>>>
>>>static const struct mfd_cell at91_usart_spi_subdev = {
>>> .name = "at91_usart_spi",
>>> - .of_compatible = "microchip,at91sam9g45-usart-spi",
>>>};
>>>
>>>static const struct mfd_cell at91_usart_serial_subdev = {
>>> .name = "atmel_usart_serial",
>>> - .of_compatible = "atmel,at91rm9200-usart-serial",
>>>};
>>>
>>>static int at91_usart_mode_probe(struct platform_device *pdev)
>>
>> [snip]
>>
>> Hi Lee, thank you for looking through our usart driver and for sharing
>> your thoughts. Removing the usage of compatible string means that for
>> similar serial/SPI IPs we would need to create new platform drivers.
>
> Why would you need to do that?
In the case we will have to support another similar IP, but with a
different set of features. Not a new platform driver from scratch, but
at least a new struct platform_driver for each variant.
>
>> This is not ideal, but it's a solution. What I proposed is more
>> flexible, but, as you pointed out, I am not sure it correctly describes
>> the HW, because the decision of whether to use this IP as a serial or a
>> SPI is a configurable one.
>>
>> Thanks and best regards,
>> Codrin
>
> --
> Lee Jones [李琼斯]
> Senior Technical Lead - Developer Services
> Linaro.org │ Open source software for Arm SoCs
> Follow Linaro: Facebook | Twitter | Blog
>
Re: [PATCH] ARM: dts: at91: add serial MFD sub-node for usart
On 02.11.2020 11:07, Lee Jones wrote:
> On Fri, 30 Oct 2020, codrin.ciubota...@microchip.com wrote:
>
>> On 30.10.2020 15:38, Nicolas Ferre wrote:
>>> On 30/10/2020 at 12:07, Codrin Ciubotariu wrote:
The "atmel,at91sam9260-usart" driver is a MFD driver, so it needs
sub-nodes
to match the registered platform device. For this reason, we add a serial
subnode to all the "atmel,at91sam9260-usart" serial compatible nods. This
will also remove the boot warning:
"atmel_usart_serial: Failed to locate of_node [id: -2]"
>>>
>>> I don't remember this warning was raised previously even if the MFD
>>> driver was added a while ago (Sept. 2018).
>>>
>>> I would say it's due to 466a62d7642f ("mfd: core: Make a best effort
>>> attempt to match devices with the correct of_nodes") which was added on
>>> mid August and corrected with 22380b65dc70 ("mfd: mfd-core: Ensure
>>> disabled devices are ignored without error") but maybe not covering our
>>> case.
>>
>> Well, it's not covering our enabled devices.
>>
>>>
>>> So, well, I don't know what's the best option to this change. Moreover,
>>> I would say that all other USART related properties go into the child
>>> not if there is a need for one.
>>>
>>> Lee, I suspect that we're not the only ones experiencing this ugly
>>> warning during the boot log: can you point us out how to deal with it
>>> for our existing atmel_serial.c users?
>>
>> My understading is that platform devices registered by MFD should have a
>> correspondig DT node. The parrent properties are also available for the
>> other usart device (usart-spi), so I think we should keep them in the
>> parrent.
>
> Device Tree and MFD are unrelated. MFDs don't even exist - they are a
> figment of a Linux Kernel Engineer's imagination - we made them up!
>
> The DT should describe the hardware and nothing else. If we wish to
> mess with devices for our own gain i.e. organise them into different
> subsystems, we have to do that in software. That's what MFD is for.
You are right, I mixed up things. We are using the MFD here to describe
a hardware USART IP that can also function as an SPI, but not at the
same time. The decision of whether the IP works as a normal USART or an
SPI is DT configurable, at this moment. It is doing more than just
describing the HW, but I don't know how to describe it otherwise.
Best regards,
Codrin
Re: [PATCH] ARM: dts: at91: add serial MFD sub-node for usart
On 02.11.2020 11:01, Lee Jones wrote:
> On Fri, 30 Oct 2020, Nicolas Ferre wrote:
>
>> On 30/10/2020 at 12:07, Codrin Ciubotariu wrote:
>>> The "atmel,at91sam9260-usart" driver is a MFD driver, so it needs sub-nodes
>>> to match the registered platform device. For this reason, we add a serial
>>> subnode to all the "atmel,at91sam9260-usart" serial compatible nods. This
>>> will also remove the boot warning:
>>> "atmel_usart_serial: Failed to locate of_node [id: -2]"
>>
>> I don't remember this warning was raised previously even if the MFD driver
>> was added a while ago (Sept. 2018).
>>
>> I would say it's due to 466a62d7642f ("mfd: core: Make a best effort attempt
>> to match devices with the correct of_nodes") which was added on mid August
>> and corrected with 22380b65dc70 ("mfd: mfd-core: Ensure disabled devices are
>> ignored without error") but maybe not covering our case.
>>
>> So, well, I don't know what's the best option to this change. Moreover, I
>> would say that all other USART related properties go into the child not if
>> there is a need for one.
>>
>> Lee, I suspect that we're not the only ones experiencing this ugly warning
>> during the boot log: can you point us out how to deal with it for our
>> existing atmel_serial.c users?
>
> You should not be instantiating drivers through Device Tree which are
> not described there. If the correct representation of the H/W already
> exists in Device Tree i.e. no SPI and UART IP really exists, use the
> MFD core API to register them utilising the platform API instead.
>
> This should do it:
>
> diff --git a/drivers/mfd/at91-usart.c b/drivers/mfd/at91-usart.c
> index 6a8351a4588e2..939bd2332a4f6 100644
> --- a/drivers/mfd/at91-usart.c
> +++ b/drivers/mfd/at91-usart.c
> @@ -17,12 +17,10 @@
>
> static const struct mfd_cell at91_usart_spi_subdev = {
> .name = "at91_usart_spi",
> - .of_compatible = "microchip,at91sam9g45-usart-spi",
> };
>
> static const struct mfd_cell at91_usart_serial_subdev = {
> .name = "atmel_usart_serial",
> - .of_compatible = "atmel,at91rm9200-usart-serial",
> };
>
> static int at91_usart_mode_probe(struct platform_device *pdev)
[snip]
Hi Lee, thank you for looking through our usart driver and for sharing
your thoughts. Removing the usage of compatible string means that for
similar serial/SPI IPs we would need to create new platform drivers.
This is not ideal, but it's a solution. What I proposed is more
flexible, but, as you pointed out, I am not sure it correctly describes
the HW, because the decision of whether to use this IP as a serial or a
SPI is a configurable one.
Thanks and best regards,
Codrin
Re: [PATCH] ARM: dts: at91: add serial MFD sub-node for usart
On 30.10.2020 15:38, Nicolas Ferre wrote:
> On 30/10/2020 at 12:07, Codrin Ciubotariu wrote:
>> The "atmel,at91sam9260-usart" driver is a MFD driver, so it needs
>> sub-nodes
>> to match the registered platform device. For this reason, we add a serial
>> subnode to all the "atmel,at91sam9260-usart" serial compatible nods. This
>> will also remove the boot warning:
>> "atmel_usart_serial: Failed to locate of_node [id: -2]"
>
> I don't remember this warning was raised previously even if the MFD
> driver was added a while ago (Sept. 2018).
>
> I would say it's due to 466a62d7642f ("mfd: core: Make a best effort
> attempt to match devices with the correct of_nodes") which was added on
> mid August and corrected with 22380b65dc70 ("mfd: mfd-core: Ensure
> disabled devices are ignored without error") but maybe not covering our
> case.
Well, it's not covering our enabled devices.
>
> So, well, I don't know what's the best option to this change. Moreover,
> I would say that all other USART related properties go into the child
> not if there is a need for one.
>
> Lee, I suspect that we're not the only ones experiencing this ugly
> warning during the boot log: can you point us out how to deal with it
> for our existing atmel_serial.c users?
My understading is that platform devices registered by MFD should have a
correspondig DT node. The parrent properties are also available for the
other usart device (usart-spi), so I think we should keep them in the
parrent.
Best regards,
Codrin
Re: [PATCH] i2c: aT91: remove legacy DMA left overs
On 30.09.2020 17:56, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Commit dc6df6e90de9 ("i2c: at91: remove legacy DMA support") removed legcy
> DMA support from the driver. Remove the last use of the definitions from
> linux/platform_data/dma-atmel.h and stop including this header.
>
> Signed-off-by: Alexandre Belloni
There is a capital 'T' in the subject, not sure it matters. Otherwise:
Reviewed-by: Codrin Ciubotariu
Thanks!
Re: [PATCH -next] i2c: at91: Use devm_platform_get_and_ioremap_resource()
On 18.09.2020 11:21, Wang ShaoBo wrote: > Make use of devm_platform_get_and_ioremap_resource() provided by > driver core platform instead of duplicated analogue. > > Signed-off-by: Wang ShaoBo Reviewed-by: Codrin Ciubotariu Thanks! > --- > drivers/i2c/busses/i2c-at91-core.c | 11 --- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-at91-core.c > b/drivers/i2c/busses/i2c-at91-core.c > index e14edd236108..5b7781302852 100644 > --- a/drivers/i2c/busses/i2c-at91-core.c > +++ b/drivers/i2c/busses/i2c-at91-core.c > @@ -207,19 +207,16 @@ static int at91_twi_probe(struct platform_device *pdev) > > dev->dev = &pdev->dev; > > - mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - if (!mem) > - return -ENODEV; > + dev->base = devm_platform_get_and_ioremap_resource(pdev, 0, &mem); > + if (IS_ERR(dev->base)) > + return PTR_ERR(dev->base); > + > phy_addr = mem->start; > > dev->pdata = at91_twi_get_driver_data(pdev); > if (!dev->pdata) > return -ENODEV; > > - dev->base = devm_ioremap_resource(&pdev->dev, mem); > - if (IS_ERR(dev->base)) > - return PTR_ERR(dev->base); > - > dev->irq = platform_get_irq(pdev, 0); > if (dev->irq < 0) > return dev->irq; > -- > 2.17.1 >
Re: [PATCH][next] ASoC: mchp-spdifrx: fix spelling mistake "overrrun" -> "overrun"
On 06.10.2020 18:20, Colin King wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > From: Colin Ian King > > There is a spelling mistake in a dev_warn message. Fix it. > > Signed-off-by: Colin Ian King Reviewed-by: Codrin Ciubotariu Thank you Colin! Best regards, Codrin
Re: [PATCH v2] i2c: pxa: move to generic GPIO recovery
On 04.10.2020 12:24, Russell King - ARM Linux admin wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Sun, Oct 04, 2020 at 12:16:56PM +0300, Codrin Ciubotariu wrote:
>> Starting with
>> commit 75820314de26 ("i2c: core: add generic I2C GPIO recovery")
>> GPIO bus recovery is supported by the I2C core, so we can remove the
>> driver implementation and use that one instead.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> This patch is not tested.
>>
>> Changes in v2:
>> - readded the pinctrl state change to default from the
>> unprepare_recovery callback;
>
> I don't think you've build tested this patch...
You're right, sorry about that. I used a wrong config... Will fix it
right away.
Best regards,
Codrin
>
>> diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
>> index 35ca2c02c9b9..006cc1d5931f 100644
>> --- a/drivers/i2c/busses/i2c-pxa.c
>> +++ b/drivers/i2c/busses/i2c-pxa.c
>> @@ -264,9 +264,6 @@ struct pxa_i2c {
>>u32 hs_mask;
>>
>>struct i2c_bus_recovery_info recovery;
>> - struct pinctrl *pinctrl;
>> - struct pinctrl_state*pinctrl_default;
>> - struct pinctrl_state*pinctrl_recovery;
>
> i2c_pxa_unprepare_recovery() refers to pinctrl and pinctrl_default which
> you've retained in this version of the patch, but you've deleted the
> members from this structure - which will lead to a build error.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
>
Re: [PATCH] i2c: pxa: move to generic GPIO recovery
>> static void i2c_pxa_unprepare_recovery(struct i2c_adapter *adap) >> @@ -1325,8 +1320,6 @@ static void i2c_pxa_unprepare_recovery(struct >> i2c_adapter *adap) >>i2c_pxa_do_reset(i2c); >>} >> >> - WARN_ON(pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default)); >> - > > This won't fly. We need to put the pinctrl back into i2c mode _before_ > we re-enable the I2C module, otherwise the I2C block will see logic 0 > on both SCL and SDA which could confuse the block. Right, I will add it back. Best regards, Codrin
Re: [PATCH net] net: dsa: microchip: ksz8795: really set the correct number of ports
On 16.09.2020 13:08, Matthias Schiffer wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> The KSZ9477 and KSZ8795 use the port_cnt field differently: For the
> KSZ9477, it includes the CPU port(s), while for the KSZ8795, it doesn't.
>
> It would be a good cleanup to make the handling of both drivers match,
> but as a first step, fix the recently broken assignment of num_ports in
> the KSZ8795 driver (which completely broke probing, as the CPU port
> index was always failing the num_ports check).
>
> Fixes: af199a1a9cb0 ("net: dsa: microchip: set the correct number of
> ports")
> Signed-off-by: Matthias Schiffer
> ---
Sorry about this. I assumed consistency between the two drivers.
Reviewed-by: Codrin Ciubotariu
Re: Re: Re: [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
On 26.08.2020 09:14, Wolfram Sang wrote: > >> Thanks, this would be great! I tested this on a sam9x60, with the HW >> feature for the 9 pulses disabled, with a picky audio codec as I2C device. >> Please let me know of the result. > > I can't make use of the feature on the platform I had in mind, sadly. It > doesn't really support switching from/to GPIO pinctrl states. If that > ever changes, I will add bus recovery for that controller, but I think > this is low priority. The pinmux driver needs to have strict set to false, otherwise the switching is not available, not at this time at least. Perhaps there is room for improvement here, because the I2C bus is not using the pins while we are doing GPIO recovery. > > On the good side, there are patches which make i2c-mv64xxx another user > of your new mechanism, so everything is well, I think. > I saw them, I will try to take a look. I am not sure I'll have time the next week to work on what you asked me regarding sh_mobile and PXA, but I will look into it the week after that. Sorry about my delayed reply, I was on vacation. Best regards, Codrin
Re: Re: [PATCH 0/4] i2c: core: add generic GPIO bus recovery
On 05.08.2020 11:52, w...@kernel.org wrote: > Codrin, everyone > >> This patch series was previously sent as a RFC. Significant changes >> since RFC: >> - "recovery" pinctrl state marked as deprecared in bindings; >> - move to "gpio" pinctrl state done after the call to prepare_recovery() >>callback; >> - glitch protection when SDA gpio is taken at initialization; > > Thanks for the fast update, now all merged for inclusion into 5.9. I > think it is really good, but to verify and double check, I think two > things would be even better.. Happy to help. > > One thing, I'll definately be doing is to add this feature to > i2c-sh_mobile.c and scope the results. > > The other thing would be to convert the PXA driver and see if our > generic support can help their advanced use case or if we are missing > something. Codrin, do you have maybe time and interest to do that? That > would be awesome! Naturally, these would be the next steps. I can do this, sure, but I don't have the HW. I'll look for some development kits. If you have any recommendations, please let me know. Best regards, Codrin
Re: [PATCH v3 2/2] ASoC: mchp-spdiftx: add driver for S/PDIF TX Controller
On 03.08.2020 20:11, Claudiu Beznea - M18063 wrote: > > > On 03.08.2020 19:11, Codrin Ciubotariu - M19940 wrote: >> On 03.08.2020 16:06, Claudiu Beznea - M18063 wrote: >>> >>> >>> On 03.08.2020 11:18, Codrin Ciubotariu wrote: The new SPDIF TX controller is a serial port compliant with the IEC- 60958 standard. It also supports programmable User Data and Channel Status fields. This IP is embedded in Microchip's sama7g5 SoC. Signed-off-by: Codrin Ciubotariu --- Changes in v2, v3: - none; sound/soc/atmel/Kconfig| 12 + sound/soc/atmel/Makefile | 2 + sound/soc/atmel/mchp-spdiftx.c | 864 + 3 files changed, 878 insertions(+) create mode 100644 sound/soc/atmel/mchp-spdiftx.c diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig index 71f2d42188c4..93beb7d670a3 100644 --- a/sound/soc/atmel/Kconfig +++ b/sound/soc/atmel/Kconfig @@ -132,4 +132,16 @@ config SND_MCHP_SOC_I2S_MCC and supports a Time Division Multiplexed (TDM) interface with external multi-channel audio codecs. +config SND_MCHP_SOC_SPDIFTX + tristate "Microchip ASoC driver for boards using S/PDIF TX" + depends on OF && (ARCH_AT91 || COMPILE_TEST) + select SND_SOC_GENERIC_DMAENGINE_PCM + select REGMAP_MMIO + help +Say Y or M if you want to add support for Microchip S/PDIF TX ASoc +driver on the following Microchip platforms: +- sama7g5 + +This S/PDIF TX driver is compliant with IEC-60958 standard and +includes programable User Data and Channel Status fields. endif diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile index c7d2989791be..3fd89a0063df 100644 --- a/sound/soc/atmel/Makefile +++ b/sound/soc/atmel/Makefile @@ -5,6 +5,7 @@ snd-soc-atmel-pcm-dma-objs := atmel-pcm-dma.o snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o snd-soc-atmel-i2s-objs := atmel-i2s.o snd-soc-mchp-i2s-mcc-objs := mchp-i2s-mcc.o +snd-soc-mchp-spdiftx-objs := mchp-spdiftx.o # pdc and dma need to both be built-in if any user of # ssc is built-in. @@ -17,6 +18,7 @@ endif obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o obj-$(CONFIG_SND_ATMEL_SOC_I2S) += snd-soc-atmel-i2s.o obj-$(CONFIG_SND_MCHP_SOC_I2S_MCC) += snd-soc-mchp-i2s-mcc.o +obj-$(CONFIG_SND_MCHP_SOC_SPDIFTX) += snd-soc-mchp-spdiftx.o # AT91 Machine Support snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o diff --git a/sound/soc/atmel/mchp-spdiftx.c b/sound/soc/atmel/mchp-spdiftx.c new file mode 100644 index ..738f6788212e --- /dev/null +++ b/sound/soc/atmel/mchp-spdiftx.c @@ -0,0 +1,864 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Driver for Microchip S/PDIF TX Controller +// +// Copyright (C) 2020 Microchip Technology Inc. and its subsidiaries +// +// Author: Codrin Ciubotariu + +#include +#include +#include +#include + +#include +#include +#include +#include + +/* + * S/PDIF Transmitter Controller Register map + */ +#define SPDIFTX_CR0x00/* Control Register */ +#define SPDIFTX_MR0x04/* Mode Register */ >>> >>> This register is read/write either in atomic and non-atomic contextes but >>> not protected everywhere the same way. >> >> I only need the TXEN bit from this register in an atomic context, this >> is why there are also non-atomic contexts. >> >>> +#define SPDIFTX_CDR 0x0C/* Common Data Register */ + +#define SPDIFTX_IER 0x14/* Interrupt Enable Register */ +#define SPDIFTX_IDR 0x18/* Interrupt Disable Register */ +#define SPDIFTX_IMR 0x1C/* Interrupt Mask Register */ +#define SPDIFTX_ISR 0x20/* Interrupt Status Register */ + +#define SPDIFTX_CH1UD(reg)(0x50 + (reg) * 4) /* User Data 1 Register x */ +#define SPDIFTX_CH1S(reg) (0x80 + (reg) * 4) /* Channel Status 1 Register x */ + +#define SPDIFTX_VERSION 0xF0 + +/* + * Control Register (Write-only) + */ +#define SPDIFTX_CR_SWRST BIT(0) /* Software Reset */ +#define SPDIFTX_CR_FCLR BIT(1) /* FIFO clear */ + +/* + * Mode Register (Read/Write) + */ +/* Transmit Enable */ +#define SPDIFTX_MR_TXEN_MASK GENMASK(0, 0) +#define SPDIFTX_MR_TXEN_DISABLE (0 << 0) +#d
Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
On 03.08.2020 17:16, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Jul 30, 2020 at 09:00:36AM +, codrin.ciubota...@microchip.com > wrote: >> On 27.07.2020 13:50, Russell King - ARM Linux admin wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>> content is safe >>> >>> On Mon, Jul 27, 2020 at 10:44:57AM +, codrin.ciubota...@microchip.com >>> wrote: On 24.07.2020 23:52, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know > the content is safe > > On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote: >> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote: >>> +- pinctrl + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus + recovery, call it "gpio" or "recovery" state >>> >>> I think we should stick with "gpio" only. That is what at91 and imx have >>> in their bindings. pxa uses "recovery" as a pinctrl state name but I >>> can't find any further use or documentation of that. PXA is not fully >>> converted to the best of my knowledge, so maybe it is no problem for PXA >>> to switch to "gpio", too? We should ask Russell King (cced). > > Fully converted to what? The generic handling where the i2c core layer > handles everything to do with recovery, including the switch between > modes? > > i2c-pxa _intentionally_ carefully handles the switch between i2c mode and > GPIO mode, and I don't see a generic driver doing that to avoid causing > any additional glitches on the bus. Given the use case that this recovery > is targetted at, avoiding glitches is very important to keep. Why is it not possbile to handle glitches in a generic way? I guess it depends on the pinctl, but we could treat a worst-case scenario to assure the switch between states is done properly. >>> >>> Please look at how i2c-pxa switches between the two, and decide whether >>> the generic implementation can do the same. >> >> The handling of glitches from initialization looks generic to me. I see >> that there are specific clear/reset routines that are in the >> (un)prepare_recovery() callbacks, but these callbacks are not replaced >> by the generic i2c recovery and will still be used if given by the >> driver. The only thing the generic recovery does is to switch the pinmux >> state. We can discuss whether we want to change the pinmux state first >> or call the (un)preapre_recovery(). > > Right, the key point i2c-pxa does is that on prepare: > - read the current state of the SCL and SDA lines and set the GPIO to >reflect those values. > - then switch the pinmux state. > > That must be preserved, otherwise if SCL is being held low by the I2C > master, and we switch to GPIO mode, SCL will be released. So the > driver needs to be involved before the pinmux state is changed. I understand, and I admit that I didn't see this case. In my mind, the master would always be in (almost) a reset state before calling for SDA recovery, so it won't hold any lines. These steps can't be generic, of course. Also, not all I2C masters have a way to show the state of its lines. For these masters, one idea would be to reset them before calling i2c_recover_bus() > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
Re: [PATCH v3 2/2] ASoC: mchp-spdiftx: add driver for S/PDIF TX Controller
On 03.08.2020 16:06, Claudiu Beznea - M18063 wrote: > > > On 03.08.2020 11:18, Codrin Ciubotariu wrote: >> The new SPDIF TX controller is a serial port compliant with the IEC- >> 60958 standard. It also supports programmable User Data and Channel >> Status fields. >> >> This IP is embedded in Microchip's sama7g5 SoC. >> >> Signed-off-by: Codrin Ciubotariu >> --- >> >> Changes in v2, v3: >> - none; >> >> sound/soc/atmel/Kconfig| 12 + >> sound/soc/atmel/Makefile | 2 + >> sound/soc/atmel/mchp-spdiftx.c | 864 + >> 3 files changed, 878 insertions(+) >> create mode 100644 sound/soc/atmel/mchp-spdiftx.c >> >> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig >> index 71f2d42188c4..93beb7d670a3 100644 >> --- a/sound/soc/atmel/Kconfig >> +++ b/sound/soc/atmel/Kconfig >> @@ -132,4 +132,16 @@ config SND_MCHP_SOC_I2S_MCC >>and supports a Time Division Multiplexed (TDM) interface with >>external multi-channel audio codecs. >> >> +config SND_MCHP_SOC_SPDIFTX >> +tristate "Microchip ASoC driver for boards using S/PDIF TX" >> +depends on OF && (ARCH_AT91 || COMPILE_TEST) >> +select SND_SOC_GENERIC_DMAENGINE_PCM >> +select REGMAP_MMIO >> +help >> + Say Y or M if you want to add support for Microchip S/PDIF TX ASoc >> + driver on the following Microchip platforms: >> + - sama7g5 >> + >> + This S/PDIF TX driver is compliant with IEC-60958 standard and >> + includes programable User Data and Channel Status fields. >> endif >> diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile >> index c7d2989791be..3fd89a0063df 100644 >> --- a/sound/soc/atmel/Makefile >> +++ b/sound/soc/atmel/Makefile >> @@ -5,6 +5,7 @@ snd-soc-atmel-pcm-dma-objs := atmel-pcm-dma.o >> snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o >> snd-soc-atmel-i2s-objs := atmel-i2s.o >> snd-soc-mchp-i2s-mcc-objs := mchp-i2s-mcc.o >> +snd-soc-mchp-spdiftx-objs := mchp-spdiftx.o >> >> # pdc and dma need to both be built-in if any user of >> # ssc is built-in. >> @@ -17,6 +18,7 @@ endif >> obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o >> obj-$(CONFIG_SND_ATMEL_SOC_I2S) += snd-soc-atmel-i2s.o >> obj-$(CONFIG_SND_MCHP_SOC_I2S_MCC) += snd-soc-mchp-i2s-mcc.o >> +obj-$(CONFIG_SND_MCHP_SOC_SPDIFTX) += snd-soc-mchp-spdiftx.o >> >> # AT91 Machine Support >> snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o >> diff --git a/sound/soc/atmel/mchp-spdiftx.c b/sound/soc/atmel/mchp-spdiftx.c >> new file mode 100644 >> index ..738f6788212e >> --- /dev/null >> +++ b/sound/soc/atmel/mchp-spdiftx.c >> @@ -0,0 +1,864 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// >> +// Driver for Microchip S/PDIF TX Controller >> +// >> +// Copyright (C) 2020 Microchip Technology Inc. and its subsidiaries >> +// >> +// Author: Codrin Ciubotariu >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +/* >> + * S/PDIF Transmitter Controller Register map >> + */ >> +#define SPDIFTX_CR 0x00/* Control Register */ >> +#define SPDIFTX_MR 0x04/* Mode Register */ > > This register is read/write either in atomic and non-atomic contextes but > not protected everywhere the same way. I only need the TXEN bit from this register in an atomic context, this is why there are also non-atomic contexts. > >> +#define SPDIFTX_CDR 0x0C/* Common Data Register */ >> + >> +#define SPDIFTX_IER 0x14/* Interrupt Enable Register */ >> +#define SPDIFTX_IDR 0x18/* Interrupt Disable Register */ >> +#define SPDIFTX_IMR 0x1C/* Interrupt Mask Register */ >> +#define SPDIFTX_ISR 0x20/* Interrupt Status Register */ >> + >> +#define SPDIFTX_CH1UD(reg) (0x50 + (reg) * 4) /* User Data 1 Register >> x */ >> +#define SPDIFTX_CH1S(reg) (0x80 + (reg) * 4) /* Channel Status 1 >> Register x */ >> + >> +#define SPDIFTX_VERSION 0xF0 >> + >> +/* >> + * Control Register (Write-only) >> + */ >> +#define SPDIFTX_CR_SWRSTBIT(0) /* Software Reset */ >> +#define SPDIFTX_CR_FCLR BIT(1) /* FIFO clear */ >> + >> +/* >> + * Mode Register (Read/Write) >> + */ >> +/* Transmit Enable */ >> +#define SPDIFTX_MR_TXEN_MASKGENMASK(0, 0) >> +#define SPDIFTX_MR_TXEN_DISABLE (0 << 0) >> +#define SPDIFTX_MR_TXEN_ENABLE (1 << 0) >> + >> +/* Multichannel Transfer */ >> +#define SPDIFTX_MR_MULTICH_MASK GENAMSK(1, 1) >> +#define SPDIFTX_MR_MULTICH_MONO (0 << 1) >> +#define SPDIFTX_MR_MULTICH_DUAL (1 << 1) >> + >> +/* Data Word Endian Mode */ >> +#define SPDIFTX_MR_ENDIAN_MASK GENMASK(2, 2) >> +#define SPDIFTX_MR_ENDIAN_LITTLE(0 << 2) >> +#define SPDIFTX_MR_ENDIAN_BIG
Re: Re: [RFC PATCH 4/4] i2c: at91: Move to generic GPIO bus recovery
On 02.08.2020 20:08, Wolfram Sang wrote: > On Fri, Jun 19, 2020 at 05:19:04PM +0300, Codrin Ciubotariu wrote: >> Make the Microchip at91 driver the first to use the generic GPIO bus >> recovery support from the I2C core and discard the driver implementation. >> >> Signed-off-by: Codrin Ciubotariu >> --- >> drivers/i2c/busses/i2c-at91-master.c | 69 ++-- >> drivers/i2c/busses/i2c-at91.h| 3 -- > > Nice diffstat! I will try using this new functionality with another > controller next week. > Thanks, this would be great! I tested this on a sam9x60, with the HW feature for the 9 pulses disabled, with a picky audio codec as I2C device. Please let me know of the result. Best regards, Codrin
Re: Re: [RFC PATCH 3/4] i2c: core: treat EPROBE_DEFER when acquiring SCL/SDA GPIOs
On 02.08.2020 20:05, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:03PM +0300, Codrin Ciubotariu wrote:
>> Even if I2C bus GPIO recovery is optional, devm_gpiod_get() can return
>> -EPROBE_DEFER, so we should at least treat that. This ends up with
>> i2c_register_adapter() to be able to return -EPROBE_DEFER.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> drivers/i2c/i2c-core-base.c | 22 --
>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index 4ee29fec4e93..f8d9f2048ca8 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -368,15 +368,16 @@ static int i2c_gpio_init_recovery(struct i2c_adapter
>> *adap)
>> return i2c_gpio_init_generic_recovery(adap);
>> }
>>
>> -static void i2c_init_recovery(struct i2c_adapter *adap)
>> +static int i2c_init_recovery(struct i2c_adapter *adap)
>> {
>> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> char *err_str;
>>
>> if (!bri)
>> -return;
>> +return 0;
>>
>> -i2c_gpio_init_recovery(adap);
>> +if (i2c_gpio_init_recovery(adap) == -EPROBE_DEFER)
>> +return -EPROBE_DEFER;
>>
>> if (!bri->recover_bus) {
>> err_str = "no recover_bus() found";
>> @@ -392,7 +393,7 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>> if (gpiod_get_direction(bri->sda_gpiod) == 0)
>> bri->set_sda = set_sda_gpio_value;
>> }
>> -return;
>> +return 0;
>
> This is correct but I think the code flow is/was confusing. Can you drop
> this 'return' and use 'else if' for the next code block? I think this is
> more readable.
Ok, it makes sense. Should I make a separate patch for this only?
One more question, should we keep:
if (!bri->set_sda && !bri->get_sda) {
err_str = "either get_sda() or set_sda() needed";
goto err;
}
?
Without {get/set}_sda we won't be able to generate stop commands and
possibly check if the bus is free, but we can still generate the SCL
clock pulses.
>
>> }
>>
>> if (bri->recover_bus == i2c_generic_scl_recovery) {
>> @@ -407,10 +408,12 @@ static void i2c_init_recovery(struct i2c_adapter *adap)
>> }
>> }
>>
>> -return;
>> +return 0;
>>err:
>> dev_err(&adap->dev, "Not using recovery: %s\n", err_str);
>> adap->bus_recovery_info = NULL;
>> +
>> +return 0;
>
> 'return -EINVAL;' I'd suggest.
OK
>
>> }
>>
>> static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>> @@ -1476,7 +1479,9 @@ static int i2c_register_adapter(struct i2c_adapter
>> *adap)
>> "Failed to create compatibility class link\n");
>> #endif
>>
>> -i2c_init_recovery(adap);
>> +res = i2c_init_recovery(adap);
>> +if (res == -EPROBE_DEFER)
>> +goto out_link;
>
> Please move 'i2c_init_recovery' above the class-link creation. It
> shouldn't make a difference but we can skip the extra label and the
> ifdeffery.
Ok. Perhaps I should also move the debug print with the registered
adapter after calling i2c_init_recovery().
>
>>
>> /* create pre-declared device nodes */
>> of_i2c_register_devices(adap);
>> @@ -1493,6 +1498,11 @@ static int i2c_register_adapter(struct i2c_adapter
>> *adap)
>>
>> return 0;
>>
>> +out_link:
>> +#ifdef CONFIG_I2C_COMPAT
>> +class_compat_remove_link(i2c_adapter_compat_class, &adap->dev,
>> + adap->dev.parent);
>> +#endif
>> out_reg:
>> init_completion(&adap->dev_released);
>> device_unregister(&adap->dev);
>> --
>> 2.25.1
>>
Do you want me to integrate this patch in the previous one?
Best regards,
Codrin
Re: Re: [RFC PATCH 2/4] i2c: core: add generic I2C GPIO recovery
On 02.08.2020 19:54, Wolfram Sang wrote:
> On Fri, Jun 19, 2020 at 05:19:02PM +0300, Codrin Ciubotariu wrote:
>> Multiple I2C bus drivers use similar bindings to obtain information needed
>> for I2C recovery. For example, for platforms using device-tree, the
>> properties look something like this:
>>
>> &i2c {
>> ...
>> pinctrl-names = "default", "gpio";
>> // or pinctrl-names = "default", "recovery";
>> pinctrl-0 = <&pinctrl_i2c_default>;
>> pinctrl-1 = <&pinctrl_i2c_gpio>;
>> sda-gpios = <&pio 0 GPIO_ACTIVE_HIGH>;
>> scl-gpios = <&pio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>> ...
>> }
>>
>> For this reason, we can add this common initialization in the core. This
>> way, other I2C bus drivers will be able to support GPIO recovery just by
>> providing a pointer to platform's pinctrl and calling i2c_recover_bus()
>> when SDA is stuck low.
>>
>> Signed-off-by: Codrin Ciubotariu
>
> Thanks, it looks a lot like what I had in mind!
>
>> ---
>> drivers/i2c/i2c-core-base.c | 119
>> include/linux/i2c.h | 11
>> 2 files changed, 130 insertions(+)
>>
>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>> index d1f278f73011..4ee29fec4e93 100644
>> --- a/drivers/i2c/i2c-core-base.c
>> +++ b/drivers/i2c/i2c-core-base.c
>> @@ -32,6 +32,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -179,6 +180,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>> struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> int i = 0, scl = 1, ret = 0;
>>
>> +if (bri->pinctrl)
>> +pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
>
> I think this should come after 'prepare_recovery'. It may be that
> 'prepare_recovery' already needs to select the pinctrl state to avoid a
> glitch. In this version, there would be a glitch then. If we move it
> down, the doubled 'pinctrl_select_state' would be a noop then.
Agree
>
>> if (bri->prepare_recovery)
>> bri->prepare_recovery(adap);
>>
>> @@ -236,6 +239,8 @@ int i2c_generic_scl_recovery(struct i2c_adapter *adap)
>>
>> if (bri->unprepare_recovery)
>> bri->unprepare_recovery(adap);
>> +if (bri->pinctrl)
>> +pinctrl_select_state(bri->pinctrl, bri->pins_default);
>
> Here it is OK and will still work with the PXA version which needs to
> select the state on its own.
>
>>
>> return ret;
>> }
>> @@ -251,6 +256,118 @@ int i2c_recover_bus(struct i2c_adapter *adap)
>> }
>> EXPORT_SYMBOL_GPL(i2c_recover_bus);
>>
>> +static void i2c_gpio_init_pinctrl_recovery(struct i2c_adapter *adap)
>> +{
>> +struct i2c_bus_recovery_info *bri = adap->bus_recovery_info;
>> +struct device *dev = &adap->dev;
>> +struct pinctrl *p = bri->pinctrl;
>> +
>> +/*
>> + * we can't change states without pinctrl, so remove the states if
>> + * available
>
> s/available/populated/ ?
OK
>
>> + */
>> +if (!p) {
>> +bri->pins_default = NULL;
>> +bri->pins_gpio = NULL;
>> +return;
>> +}
>> +
>> +if (!bri->pins_default) {
>> +bri->pins_default = pinctrl_lookup_state(p,
>> + PINCTRL_STATE_DEFAULT);
>> +if (IS_ERR(bri->pins_default)) {
>> +dev_dbg(dev, PINCTRL_STATE_DEFAULT " state not found
>> for GPIO recovery\n");
>> +bri->pins_default = NULL;
>> +
>> +goto cleanup_pinctrl;
>
> I'd leave out the goto here. It is OK to check both parameters, I think.
since default state is missing, the next check can be skipped, but I
agree that removing the label makes things easier to read.
>
>> +}
>> +}
>> +if (!bri->pins_gpio) {
>> +bri->pins_gpio = pinctrl_lookup_state(p, "gpio");
>> +if (IS_ERR(bri->pins_gpio))
>> +bri->pins_gpio = pinctrl_lookup_state(p, "recovery");
>> +
>> +if (IS_ERR(bri->pins_gpio)) {
>> +dev_dbg(dev, "no gpio or recovery state found for GPIO
>> recovery\n");
>> +bri->pins_gpio = NULL;
>> +
>> +goto cleanup_pinctrl;
>
> This goto is not needed...
right
>
>> +}
>> +}
>> +
>> +cleanup_pinctrl:
>
> ... and this label can go then. Also nicer to read, I'd say.
I will remove it.
>
>> +/* for pinctrl state changes, we need all the information */
>> +if (!bri->pins_default || !bri->pins_gpio) {
>> +bri->pinctrl = NULL;
>> +bri->pins_default = NULL;
>> +bri->pins_gpio = NULL;
>> +} else {
>> +dev_info(dev, "using pinctrl states for GPIO recovery");
>> +}
>> +}
>> +
>> +static int i2c_gpio_init_generic_recovery(struct i2c_adapter *adap)
>> +{
>> +struct i2c_bus_recovery_info *bri = adap->b
Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
On 27.07.2020 13:50, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Mon, Jul 27, 2020 at 10:44:57AM +, codrin.ciubota...@microchip.com > wrote: >> On 24.07.2020 23:52, Russell King - ARM Linux admin wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the >>> content is safe >>> >>> On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote: On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote: > >> +- pinctrl >> + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus >> + recovery, call it "gpio" or "recovery" state > > I think we should stick with "gpio" only. That is what at91 and imx have > in their bindings. pxa uses "recovery" as a pinctrl state name but I > can't find any further use or documentation of that. PXA is not fully > converted to the best of my knowledge, so maybe it is no problem for PXA > to switch to "gpio", too? We should ask Russell King (cced). >>> >>> Fully converted to what? The generic handling where the i2c core layer >>> handles everything to do with recovery, including the switch between >>> modes? >>> >>> i2c-pxa _intentionally_ carefully handles the switch between i2c mode and >>> GPIO mode, and I don't see a generic driver doing that to avoid causing >>> any additional glitches on the bus. Given the use case that this recovery >>> is targetted at, avoiding glitches is very important to keep. >> >> Why is it not possbile to handle glitches in a generic way? I guess it >> depends on the pinctl, but we could treat a worst-case scenario to >> assure the switch between states is done properly. > > Please look at how i2c-pxa switches between the two, and decide whether > the generic implementation can do the same. The handling of glitches from initialization looks generic to me. I see that there are specific clear/reset routines that are in the (un)prepare_recovery() callbacks, but these callbacks are not replaced by the generic i2c recovery and will still be used if given by the driver. The only thing the generic recovery does is to switch the pinmux state. We can discuss whether we want to change the pinmux state first or call the (un)preapre_recovery(). What I had in mind for the generic recovery was to just handle the common parts that follow the same bindings, which is getting the gpios and changing the pinmux states before recovering. Best regards, Codrin > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
Re: [RFC PATCH 1/4] dt-binding: i2c: add generic properties for GPIO bus recovery
On 24.07.2020 23:52, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Fri, Jul 24, 2020 at 09:39:13PM +0200, Wolfram Sang wrote: >> On Sun, Jul 05, 2020 at 11:19:18PM +0200, Wolfram Sang wrote: >>> +- pinctrl + add extra pinctrl to configure SCL/SDA pins to GPIO function for bus + recovery, call it "gpio" or "recovery" state >>> >>> I think we should stick with "gpio" only. That is what at91 and imx have >>> in their bindings. pxa uses "recovery" as a pinctrl state name but I >>> can't find any further use or documentation of that. PXA is not fully >>> converted to the best of my knowledge, so maybe it is no problem for PXA >>> to switch to "gpio", too? We should ask Russell King (cced). > > Fully converted to what? The generic handling where the i2c core layer > handles everything to do with recovery, including the switch between > modes? > > i2c-pxa _intentionally_ carefully handles the switch between i2c mode and > GPIO mode, and I don't see a generic driver doing that to avoid causing > any additional glitches on the bus. Given the use case that this recovery > is targetted at, avoiding glitches is very important to keep. Why is it not possbile to handle glitches in a generic way? I guess it depends on the pinctl, but we could treat a worst-case scenario to assure the switch between states is done properly. > >>> Russell, do you object naming the pinctrl state for bus recovery in >>> the pxa i2c driver from "recovery" to "gpio"? >> >> No response, so far. I suggest now to support the "recovery" naming but >> mark it as deprecated. Opinions? > > I don't have a preference on the exact naming. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
Re: [PATCH net-next v3 3/7] net: macb: parse PHY nodes found under an MDIO node
On 24.07.2020 20:40, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/24/20 3:50 AM, Codrin Ciubotariu wrote:
>> The MACB embeds an MDIO bus controller. For this reason, the PHY nodes
>> were represented as sub-nodes in the MACB node. Generally, the
>> Ethernet controller is different than the MDIO controller, so the PHYs
>> are probed by a separate MDIO driver. Since adding the PHY nodes directly
>> under the ETH node became deprecated, we adjust the MACB driver to look
>> for an MDIO node and register the subnode MDIO devices.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> Changes in v3:
>> - moved the check for the mdio node at the beginnging of
>> macb_mdiobus_register(). This way, the mdio devices will be probed even
>> if macb is a fixed-link
>>
>> Changes in v2:
>> - readded newline removed by mistake;
>>
>> drivers/net/ethernet/cadence/macb_main.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 89fe7af5e408..cb0b3637651c 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,6 +740,16 @@ static int macb_mii_probe(struct net_device *dev)
>> static int macb_mdiobus_register(struct macb *bp)
>> {
>>struct device_node *child, *np = bp->pdev->dev.of_node;
>> + struct device_node *mdio_node;
>> + int ret;
>> +
>> + /* if an MDIO node is present, it should contain the PHY nodes */
>> + mdio_node = of_get_child_by_name(np, "mdio");
>> + if (mdio_node) {
>> + ret = of_mdiobus_register(bp->mii_bus, mdio_node);
>> + of_node_put(mdio_node);
>> + return ret;
>> + }
>
> This does take care of registering the MDIO bus controller when present
> as a sub-node, however if you also plan on making use of fixed-link, we
> will have already returned.
>
>>
>>if (of_phy_is_fixed_link(np))
>>return mdiobus_register(bp->mii_bus);
>>
>
> Really not sure what this is achieving, because we start off assuming
> that we have an OF driven configuration, but later on we register the
> MDIO bus with mdiobus_register() (and not of_mdiobus_register()), so no
> scanning of the MDIO bus will happen.
I agree that returning mdiobus_register() here makes no sense since
there are no other PHY devices to scan for and probe. I can replace this
with NULL.
The reason for fixed-link check here is to skip the following loop:
for_each_available_child_of_node(np, child)
if (of_mdiobus_child_is_phy(child)) {
of_node_put(child);
return of_mdiobus_register(bp->mii_bus, np);
}
of_mdiobus_child_is_phy() returns true when it finds the fixed-link
node, because it has no compatible.
>
> How does the driver currently support being provided a fixed-link
> property? Should not we at least have this pattern:
>
> */
> if (of_phy_is_fixed_link(dn)) {
> ret = of_phy_register_fixed_link(dn);
> if (ret)
> return ret;
>
> priv->phy_dn = dn;
> }
>
> It does not look like you are breaking anything here, because it does
> not look like this works at all.
From what I understand, the new phylink(_create) handles the fixed-link
case, so there is not reason to explicitly register the fixed-link.
Best regards,
Codrin
> --
> Florian
>
Re: [PATCH net-next v2 3/7] net: macb: parse PHY nodes found under an MDIO node
On 23.07.2020 21:59, Florian Fainelli wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 7/21/20 10:13 AM, Codrin Ciubotariu wrote:
>> The MACB embeds an MDIO bus controller. For this reason, the PHY nodes
>> were represented as sub-nodes in the MACB node. Generally, the
>> Ethernet controller is different than the MDIO controller, so the PHYs
>> are probed by a separate MDIO driver. Since adding the PHY nodes directly
>> under the ETH node became deprecated, we adjust the MACB driver to look
>> for an MDIO node and register the subnode MDIO devices.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> Changes in v2:
>> - readded newline removed by mistake;
>>
>> drivers/net/ethernet/cadence/macb_main.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 89fe7af5e408..b25c64b45148 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -740,10 +740,20 @@ static int macb_mii_probe(struct net_device *dev)
>> static int macb_mdiobus_register(struct macb *bp)
>> {
>>struct device_node *child, *np = bp->pdev->dev.of_node;
>> + struct device_node *mdio_node;
>> + int ret;
>>
>>if (of_phy_is_fixed_link(np))
>>return mdiobus_register(bp->mii_bus);
>
> Does not this need changing as well? Consider the use case of having
> your MACB Ethernet node have a fixed-link property to describe how it
> connects to a switch, and your MACB MDIO controller, expressed as a
> sub-node, describing the MDIO attached switch it connects to.
Right, this is what I was discussing with Claudiu on the other thread. I
am thinking to just move the look for mdio before checking for
fixed-link. This will probe the MDIO devices and simple mdiobus_register
will be called only if the mdio node is missing.
Thank you for your review(s)!
Best regards,
Codrin
>
>>
>> + /* if an MDIO node is present, it should contain the PHY nodes */
>> + mdio_node = of_get_child_by_name(np, "mdio");
>> + if (mdio_node) {
>> + ret = of_mdiobus_register(bp->mii_bus, mdio_node);
>> + of_node_put(mdio_node);
>> + return ret;
>> + }
>> +
>>/* Only create the PHY from the device tree if at least one PHY is
>> * described. Otherwise scan the entire MDIO bus. We do this to
>> support
>> * old device tree that did not follow the best practices and did not
>>
>
>
> --
> Florian
>
Re: [PATCH net-next v2 0/7] Add an MDIO sub-node under MACB
On 23.07.2020 10:51, Claudiu Beznea - M18063 wrote:
>
>
> On 22.07.2020 14:38, Codrin Ciubotariu - M19940 wrote:
>> On 22.07.2020 13:32, Claudiu Beznea - M18063 wrote:
>>>
>>>
>>> On 21.07.2020 20:13, Codrin Ciubotariu wrote:
Adding the PHY nodes directly under the Ethernet node became deprecated,
so the aim of this patch series is to make MACB use an MDIO node as
container for MDIO devices.
This patch series starts with a small patch to use the device-managed
devm_mdiobus_alloc(). In the next two patches we update the bindings and
adapt macb driver to parse the device-tree PHY nodes from under an MDIO
node. The last patches add the MDIO node in the device-trees of sama5d2,
sama5d3, samad4 and sam9x60 boards.
>>>
>>> Tested this series on sama5d2_xplained in the following scenarios:
>>>
>>> 1/ PHY bindings from patch 4/7:
>>> mdio {
>>> #address-cells = <1>;
>>> #size-cells = <0>;
>>> ethernet-phy@1 {
>>> reg = <0x1>;
>>> interrupt-parent = <&pioA>;
>>> interrupts = ;
>>> };
>>>
>>> 2/ PHY bindings before this series:
>>> ethernet-phy@1 {
>>> reg = <0x1>;
>>> interrupt-parent = <&pioA>;
>>> interrupts = ;
>>> };
>>>
>>> 3/ No PHY bindings at all.
>>>
>>> All 3 cases went OK.
>>>
>>> You can add:
>>> Tested-by: Claudiu Beznea
>>> Acked-by: Claudiu Beznea
>>
>> Thank you very much Claudiu!
>> There is still one more case in my mind. macb could be a fixed-link with
>> an MDIO DSA switch. While the macb would have a fixed connection with a
>> port from the DSA switch, the switch could be configured using macb's
>> MDIO. The dt would be something like:
>>
>> macb {
>> fixed-link {
>> ...
>> };
>> mdio {
>> switch@0 {
>> ...
>> };
>> };
>> };
>
> Do you have a setup for testing this? At the moment I don't know a
> configuration like this that macb is working with.
There isn't one that I am aware of, but we should address it.
>
>>
>> To support this, in patch 3/7 I should first check for the mdio node to
>> return of_mdiobus_register() and then check if it's a fixed-link to
>> return simple mdiobus_register(). I will address this in v3...>
>> Thanks and best regards,
>> Codrin
>>
>>>
>>> Thank you,
>>> Claudiu Beznea
>>>
Changes in v2:
- renamed patch 2/7 from "macb: bindings doc: use an MDIO node as a
container for PHY nodes" to "dt-bindings: net: macb: use an MDIO
node as a container for PHY nodes"
- added back a newline removed by mistake in patch 3/7
Codrin Ciubotariu (7):
net: macb: use device-managed devm_mdiobus_alloc()
dt-bindings: net: macb: use an MDIO node as a container for PHY nodes
net: macb: parse PHY nodes found under an MDIO node
ARM: dts: at91: sama5d2: add an mdio sub-node to macb
ARM: dts: at91: sama5d3: add an mdio sub-node to macb
ARM: dts: at91: sama5d4: add an mdio sub-node to macb
ARM: dts: at91: sam9x60: add an mdio sub-node to macb
Documentation/devicetree/bindings/net/macb.txt | 15 ---
arch/arm/boot/dts/at91-sam9x60ek.dts | 8 ++--
arch/arm/boot/dts/at91-sama5d27_som1.dtsi | 16 ++--
arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi| 17 ++---
arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 13 -
arch/arm/boot/dts/at91-sama5d2_xplained.dts| 12
arch/arm/boot/dts/at91-sama5d3_xplained.dts| 16
arch/arm/boot/dts/at91-sama5d4_xplained.dts| 12
drivers/net/ethernet/cadence/macb_main.c | 18 --
9 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH net-next v2 0/7] Add an MDIO sub-node under MACB
On 22.07.2020 13:32, Claudiu Beznea - M18063 wrote:
>
>
> On 21.07.2020 20:13, Codrin Ciubotariu wrote:
>> Adding the PHY nodes directly under the Ethernet node became deprecated,
>> so the aim of this patch series is to make MACB use an MDIO node as
>> container for MDIO devices.
>> This patch series starts with a small patch to use the device-managed
>> devm_mdiobus_alloc(). In the next two patches we update the bindings and
>> adapt macb driver to parse the device-tree PHY nodes from under an MDIO
>> node. The last patches add the MDIO node in the device-trees of sama5d2,
>> sama5d3, samad4 and sam9x60 boards.
>>
>
> Tested this series on sama5d2_xplained in the following scenarios:
>
> 1/ PHY bindings from patch 4/7:
> mdio {
> #address-cells = <1>;
> #size-cells = <0>;
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioA>;
> interrupts = ;
> };
>
> 2/ PHY bindings before this series:
> ethernet-phy@1 {
> reg = <0x1>;
> interrupt-parent = <&pioA>;
> interrupts = ;
> };
>
> 3/ No PHY bindings at all.
>
> All 3 cases went OK.
>
> You can add:
> Tested-by: Claudiu Beznea
> Acked-by: Claudiu Beznea
Thank you very much Claudiu!
There is still one more case in my mind. macb could be a fixed-link with
an MDIO DSA switch. While the macb would have a fixed connection with a
port from the DSA switch, the switch could be configured using macb's
MDIO. The dt would be something like:
macb {
fixed-link {
...
};
mdio {
switch@0 {
...
};
};
};
To support this, in patch 3/7 I should first check for the mdio node to
return of_mdiobus_register() and then check if it's a fixed-link to
return simple mdiobus_register(). I will address this in v3...
Thanks and best regards,
Codrin
>
> Thank you,
> Claudiu Beznea
>
>> Changes in v2:
>> - renamed patch 2/7 from "macb: bindings doc: use an MDIO node as a
>> container for PHY nodes" to "dt-bindings: net: macb: use an MDIO
>> node as a container for PHY nodes"
>> - added back a newline removed by mistake in patch 3/7
>>
>> Codrin Ciubotariu (7):
>>net: macb: use device-managed devm_mdiobus_alloc()
>>dt-bindings: net: macb: use an MDIO node as a container for PHY nodes
>>net: macb: parse PHY nodes found under an MDIO node
>>ARM: dts: at91: sama5d2: add an mdio sub-node to macb
>>ARM: dts: at91: sama5d3: add an mdio sub-node to macb
>>ARM: dts: at91: sama5d4: add an mdio sub-node to macb
>>ARM: dts: at91: sam9x60: add an mdio sub-node to macb
>>
>> Documentation/devicetree/bindings/net/macb.txt | 15 ---
>> arch/arm/boot/dts/at91-sam9x60ek.dts | 8 ++--
>> arch/arm/boot/dts/at91-sama5d27_som1.dtsi | 16 ++--
>> arch/arm/boot/dts/at91-sama5d27_wlsom1.dtsi| 17 ++---
>> arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 13 -
>> arch/arm/boot/dts/at91-sama5d2_xplained.dts| 12
>> arch/arm/boot/dts/at91-sama5d3_xplained.dts| 16
>> arch/arm/boot/dts/at91-sama5d4_xplained.dts| 12
>> drivers/net/ethernet/cadence/macb_main.c | 18 --
>> 9 files changed, 86 insertions(+), 41 deletions(-)
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH net-next 2/7] macb: bindings doc: use an MDIO node as a container for PHY nodes
On 21.07.2020 16:29, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi,
>
> The proper subject prefix is dt-bindings: net: macb:
Will fix in v2. Thanks!
>
> On 21/07/2020 13:02:29+0300, Codrin Ciubotariu wrote:
>> The MACB driver embeds an MDIO bus controller and for this reason there
>> was no need for an MDIO sub-node present to contain the PHY nodes. Adding
>> MDIO devies directly under an Ethernet node is deprecated, so an MDIO node
>> is included to contain of the PHY nodes (and other MDIO devices' nodes).
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> Documentation/devicetree/bindings/net/macb.txt | 15 ---
>> 1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/macb.txt
>> b/Documentation/devicetree/bindings/net/macb.txt
>> index 0b61a90f1592..88d5199c2279 100644
>> --- a/Documentation/devicetree/bindings/net/macb.txt
>> +++ b/Documentation/devicetree/bindings/net/macb.txt
>> @@ -32,6 +32,11 @@ Required properties:
>> The MAC address will be determined using the optional properties
>> defined in ethernet.txt.
>>
>> +Optional subnodes:
>> +- mdio : specifies the MDIO bus in the MACB, used as a container for PHY
>> nodes or other
>> + nodes of devices present on the MDIO bus. Please see ethernet-phy.yaml in
>> the same
>> + directory for more details.
>> +
>> Optional properties for PHY child node:
>> - reset-gpios : Should specify the gpio for phy reset
>> - magic-packet : If present, indicates that the hardware supports waking
>> @@ -48,8 +53,12 @@ Examples:
>>local-mac-address = [3a 0e 03 04 05 06];
>>clock-names = "pclk", "hclk", "tx_clk";
>>clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
>> - ethernet-phy@1 {
>> - reg = <0x1>;
>> - reset-gpios = <&pioE 6 1>;
>> + mdio {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + ethernet-phy@1 {
>> + reg = <0x1>;
>> + reset-gpios = <&pioE 6 1>;
>> + };
>>};
>>};
>> --
>> 2.25.1
>>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH net-next 3/7] net: macb: parse PHY nodes found under an MDIO node
On 21.07.2020 16:36, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > >> @@ -755,7 +765,6 @@ static int macb_mdiobus_register(struct macb *bp) >> * decrement it before returning. >> */ >>of_node_put(child); >> - >>return of_mdiobus_register(bp->mii_bus, np); >>} > > Please avoid white space changes like this. Sorry about this, it was not intended. Will fix in v2. Thanks! > > Otherwise this looks O.K. > > Andrew >
Re: [PATCH 11/19] clk: at91: clk-generated: pass the id of changeable parent at registration
Hi Claudiu,
On 15.07.2020 14:24, Claudiu Beznea wrote:
> Pass the ID of changeable parent at registration. This will allow
> the scalability of this clock driver with regards to the changeable
> parent ID for versions of this IP where changeable parent is not the
> last one in the parents list (e.g. SAMA7G5). In
> clk_generated_best_diff() the *best_diff variable is check against
> tmp_diff variable using ">=" operator instead of ">" so that in case
> the requested frequency could be obtained using fix parents + gck
> dividers but the clock also supports changeable parent to be able
> to force the usage of the changeable parent.
This is a great feature!
>
> Signed-off-by: Claudiu Beznea
> ---
> drivers/clk/at91/clk-generated.c | 26 ++
> drivers/clk/at91/dt-compat.c | 8 +---
> drivers/clk/at91/pmc.h | 4 ++--
> drivers/clk/at91/sam9x60.c | 3 +--
> drivers/clk/at91/sama5d2.c | 31 +++
> 5 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/clk/at91/clk-generated.c
> b/drivers/clk/at91/clk-generated.c
> index 2448bdc63425..f9ca04c97128 100644
> --- a/drivers/clk/at91/clk-generated.c
> +++ b/drivers/clk/at91/clk-generated.c
> @@ -18,8 +18,6 @@
>
> #define GENERATED_MAX_DIV 255
>
> -#define GCK_INDEX_DT_AUDIO_PLL 5
> -
> struct clk_generated {
> struct clk_hw hw;
> struct regmap *regmap;
> @@ -29,7 +27,7 @@ struct clk_generated {
> u32 gckdiv;
> const struct clk_pcr_layout *layout;
> u8 parent_id;
> - bool audio_pll_allowed;
> + int chg_pid;
> };
>
> #define to_clk_generated(hw) \
> @@ -109,7 +107,7 @@ static void clk_generated_best_diff(struct
> clk_rate_request *req,
> tmp_rate = parent_rate / div;
> tmp_diff = abs(req->rate - tmp_rate);
>
> - if (*best_diff < 0 || *best_diff > tmp_diff) {
> + if (*best_diff < 0 || *best_diff >= tmp_diff) {
> *best_rate = tmp_rate;
> *best_diff = tmp_diff;
> req->best_parent_rate = parent_rate;
> @@ -129,7 +127,10 @@ static int clk_generated_determine_rate(struct clk_hw
> *hw,
> int i;
> u32 div;
>
> - for (i = 0; i < clk_hw_get_num_parents(hw) - 1; i++) {
> + for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
> + if (gck->chg_pid == i)
> + continue;
> +
One thing that the previous loop was preventing was to not allow other
gcks take clk_hw_get_num_parents(hw) - 1 as a parent. So the audio pll
(last one) was reserved for the gck of the audio peripherals only. With
this change, any peripheral can use chg_pid as a parent, preventing thus
its correct use by the peripherals that can actually need and change the
rate of chg_pid.
> parent = clk_hw_get_parent_by_index(hw, i);
> if (!parent)
> continue;
> @@ -161,10 +162,10 @@ static int clk_generated_determine_rate(struct clk_hw
> *hw,
>* that the only clks able to modify gck rate are those of audio IPs.
>*/
The above comment should be updated.
>
> - if (!gck->audio_pll_allowed)
> + if (gck->chg_pid < 0)
> goto end;
Best regards,
Codrin
Re: [PATCH net] net: dsa: microchip: set the correct number of ports
On 02.07.2020 16:50, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Thu, Jul 02, 2020 at 12:44:50PM +0300, Codrin Ciubotariu wrote:
>> The number of ports is incorrectly set to the maximum available for a DSA
>> switch. Even if the extra ports are not used, this causes some functions
>> to be called later, like port_disable() and port_stp_state_set(). If the
>> driver doesn't check the port index, it will end up modifying unknown
>> registers.
>>
>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>> Signed-off-by: Codrin Ciubotariu
>
> Reviewed-by: Andrew Lunn
>
> Thanks for the minimum patch.
>
> If you wait about a week, net will get merged into net-next. You can
> then submit a refactoring patch based on your previous version.
>
> Andrew
>
Sure thing. This small version does the job, so I will see about the
refactoring, maybe I will group it with something else...
Thank you for your review!
Best regards,
Codrin
Re: [PATCH net-next] net: dsa: microchip: split adjust_link() in phylink_mac_link_{up|down}()
On 02.07.2020 13:19, Russell King - ARM Linux admin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the > content is safe > > On Thu, Jul 02, 2020 at 12:54:39PM +0300, Codrin Ciubotariu wrote: >> The DSA subsystem moved to phylink and adjust_link() became deprecated in >> the process. This patch removes adjust_link from the KSZ DSA switches and >> adds phylink_mac_link_up() and phylink_mac_link_down(). >> >> Signed-off-by: Codrin Ciubotariu > > For the code _transformation_ that the patch does: > > Reviewed-by: Russell King > > as it is equivalent. However, for a deeper review of what is going > on here, I've a question: > > $ grep live_ports * > ksz8795.c: dev->live_ports = dev->host_mask; > ksz8795.c: dev->live_ports |= BIT(port); > ksz_common.h: u16 live_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz_common.c: dev->live_ports |= (1 << port) & dev->on_ports; > ksz_common.c: dev->live_ports &= ~(1 << port); > ksz9477.c: dev->live_ports = dev->host_mask; > ksz9477.c: dev->live_ports |= (1 << port); > > These are the only references to dev->live_ports, so the purpose of > dev->live_ports seems unclear; it seems it is only updated but never > read. Can it be removed, along with the locking in your new functions? Sure, I'll make a patch to clean things up. I will resend this patch in a series to mark the dependency. Thanks and best regards, Codrin > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! >
Re: [PATCH 1/2] net: dsa: microchip: set the correct number of ports in dsa_switch
On 01.07.2020 20:09, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Wed, Jul 01, 2020 at 07:51:27PM +0300, Codrin Ciubotariu wrote:
>> The number of ports is incorrectly set to the maximum available for a DSA
>> switch. Even if the extra ports are not used, this causes some functions
>> to be called later, like port_disable() and port_stp_state_set(). If the
>> driver doesn't check the port index, it will end up modifying unknown
>> registers.
>>
>> Fixes: b987e98e50ab ("dsa: add DSA switch driver for Microchip KSZ9477")
>
> Hi Codrin
>
> You don't indicate which tree this is for. net-next, or net? It looks
> like it fixes a real issue, so it probably should be for net. But
> patches to net should be minimal. Is it possible to do the
>
> ds->num_ports = swdev->port_cnt;
>
> without all the other changes? You can then have a refactoring patch
> in net-next.
This one should be for net. Ok then, I will send a simpler version of
this patch for net, just to fix the issue and another one like this one
for net-next.
Thanks and best regards,
Codrin
>
> Thanks
> Andrew
>
Re: [PATCH 3/3] Revert "ARM: at91/dt: sama5d2 Xplained: add pdmic node"
On 18.06.2020 00:34, Alexandre Belloni wrote:
> Hi,
>
> The correct subject line prefix is "ARM: dts: at91:"
I just reverted the original patch. I can make it a normal commit if you
want.
>
> On 15/06/2020 12:55:25+0300, Codrin Ciubotariu wrote:
>> There are no PDM microphones on SAMA5D2 Xplained, to exercize the
>> PDMIC.
>>
>> This reverts commit ca6349a8c51f2e3d6f2acdb36431e7d7328261f7.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> arch/arm/boot/dts/at91-sama5d2_xplained.dts | 16
>> 1 file changed, 16 deletions(-)
>>
>
> This patch doesn't apply and I think you'll have to motivate the removal
> a bit more because this seems like a change of policy to me.
The PDMIC needs PDM microphones to work. sama5d2 xplained doesn't have
such microphones, so there is no reason to enable PDMIC and take some
pins since there is no-one using them. Isn't it the policy to enable
only what is present on a board?
>
>> diff --git a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
>> b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
>> index 54d96649da77..c0a255bda477 100644
>> --- a/arch/arm/boot/dts/at91-sama5d2_xplained.dts
>> +++ b/arch/arm/boot/dts/at91-sama5d2_xplained.dts
>> @@ -109,16 +109,6 @@ timer1: timer@1 {
>>};
>>};
>>
>> - pdmic@f8018000 {
>> - pinctrl-names = "default";
>> - pinctrl-0 = <&pinctrl_pdmic_default>;
>> - atmel,model = "PDMIC @ sama5d2_xplained";
>> - atmel,mic-min-freq = <100>;
>> - atmel,mic-max-freq = <3246000>;
>> - atmel,mic-offset = <0x0>;
>> - status = "okay";
>> - };
>> -
>>uart1: serial@f802 {
>>pinctrl-names = "default";
>>pinctrl-0 = <&pinctrl_uart1_default>;
>> @@ -533,12 +523,6 @@ pinctrl_macb0_phy_irq: macb0_phy_irq {
>>bias-disable;
>>};
>>
>> - pinctrl_pdmic_default: pdmic_default {
>> - pinmux = ,
>> - ;
>> - bias-disable;
>> - };
>> -
>>pinctrl_sdmmc0_default: sdmmc0_default {
>>cmd_data {
>>pinmux =
>> ,
>> --
>> 2.25.1
>>
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Re: Re: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery
On 20.05.2020 19:27, Wolfram Sang wrote: > >>> This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two >>> pinctrl_state pointers into bus_recovery_info and handle all this in the >>> core. I will try this later this week if noone is super-eager to try it >>> out before. >>> >> >> By 'all this' you mean to move the entire function in the core, right? >> Having just these two pointers bus_recinovery_info won't help much. I >> can try it, if you haven't already started... > > I mean to add those two pointers to bus_recinovery_info and if they are > populated, then the I2C core is doing the necessary magic (or maybe just > the pinctrl handle and assume the states have fixed names?). Russell > just sent patches to add it to the PXA driver, so we could now double > check how much could be factored out. > > I haven't started yet, let's keep in touch who started first :) > I started working at this. I added the pinctrl state initialization at the beginning of the i2c_init_recovery(). Due to the pinmux state issue with the GPIOs, the GPIO part needs to be also moved. The problem I ran in to now is the fact that, even if we can ignore if the GPIOs are not available, we should at least treat EPROBE_DEFER error. To do this, the I2C bus drivers should take into account the fact that i2c_register_adapter() can return -EPROBE_DEFER. Is this something to consider? Thanks and best regards, Codrin
Re: [PATCH] ARM: dts: at91: Configure I2C SCL gpio as open drain
On 15.05.2020 17:58, Alexandre Belloni wrote:
> On 15/05/2020 17:00:01+0300, Codrin Ciubotariu wrote:
>> The SCL gpio pin used by I2C bus for recovery needs to be configured as
>> open drain.
>>
>> Fixes: 455fec938bbb ("ARM: dts: at91: sama5d2: add i2c gpio pinctrl")
>> Fixes: a4bd8da893a3 ("ARM: dts: at91: sama5d3: add i2c gpio pinctrl")
>> Fixes: 8fb82f050cf6 ("ARM: dts: at91: sama5d4: add i2c gpio pinctrl")
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> arch/arm/boot/dts/at91-sama5d2_ptc_ek.dts | 6 +++---
>> arch/arm/boot/dts/at91-sama5d2_xplained.dts | 6 +++---
>> arch/arm/boot/dts/sama5d3.dtsi | 6 +++---
>> arch/arm/boot/dts/sama5d4.dtsi | 6 +++---
>> 4 files changed, 12 insertions(+), 12 deletions(-)
>>
>
> Applied, thanks. There was a small conflict in the sama5d2 board dts,
> please check.
It is ok, with the exception that it should also be added for the
scl-gpios property from the i2c2 node. I am making a patch.
>
> --
> Alexandre Belloni, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
Re: Re: [RFC PATCH] i2c: at91: Fix pinmux after devm_gpiod_get() for bus recovery
On 05.05.2020 18:12, Wolfram Sang wrote:
> On Wed, Apr 15, 2020 at 10:06:43AM +0300, Codrin Ciubotariu wrote:
>> devm_gpiod_get() usually calls gpio_request_enable() for non-strict pinmux
>> drivers. These puts the pins in GPIO mode, whithout notifying the pinctrl
>> driver. At this point, the I2C bus no longer owns the pins. To mux the
>> pins back to the I2C bus, we use the pinctrl driver to change the state
>> of the pins to GPIO, before using devm_gpiod_get(). After the pins are
>> received as GPIOs, we switch theer pinctrl state back to the default
>> one,
>>
>> Fixes: d3d3fdcc4c90 ("i2c: at91: implement i2c bus recovery")
>> Signed-off-by: Codrin Ciubotariu
>
> Applied to for-current, thanks!
Just looked at this patch and noticed that I should change the pinctrl
state back to default if we can't get the gpio pins. I will submit a
patch to fix this.
>
> This will do for 5.7. For 5.8 or 5.9, I can imagine to take the two
> pinctrl_state pointers into bus_recovery_info and handle all this in the
> core. I will try this later this week if noone is super-eager to try it
> out before.
>
By 'all this' you mean to move the entire function in the core, right?
Having just these two pointers bus_recinovery_info won't help much. I
can try it, if you haven't already started...
Best regards,
Codrin
Re: [PATCH] i2c: at91: Send bus clear command if SCL or SDA is down
On 19.09.2019 18:06, kbouhara wrote:
>
> On 9/11/19 11:58 AM, Codrin Ciubotariu wrote:
>> After a transfer timeout, some faulty I2C slave devices might hold down
>> the SCL or the SDA pins. We can generate a bus clear command, hoping that
>> the slave might release the pins.
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> drivers/i2c/busses/i2c-at91-master.c | 20
>> drivers/i2c/busses/i2c-at91.h | 6 +-
>> 2 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-at91-master.c
>> b/drivers/i2c/busses/i2c-at91-master.c
>> index a3fcc35ffd3b..5f544a16db96 100644
>> --- a/drivers/i2c/busses/i2c-at91-master.c
>> +++ b/drivers/i2c/busses/i2c-at91-master.c
>> @@ -599,6 +599,26 @@ static int at91_do_twi_transfer(struct
>> at91_twi_dev *dev)
>> at91_twi_write(dev, AT91_TWI_CR,
>> AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
>> }
>> +
>> + /*
>> + * After timeout, some faulty I2C slave devices might hold
>> SCL/SDA down;
>> + * we can send a bus clear command, hoping that the pins will be
>> + * released
>> + */
>> + if (!(dev->transfer_status & AT91_TWI_SDA) ||
>> + !(dev->transfer_status & AT91_TWI_SCL)) {
>> + dev_dbg(dev->dev,
>> + "SDA/SCL are down; sending bus clear command\n");
>> + if (dev->use_alt_cmd) {
>> + unsigned int acr;
>> +
>> + acr = at91_twi_read(dev, AT91_TWI_ACR);
>> + acr &= ~AT91_TWI_ACR_DATAL_MASK;
>> + at91_twi_write(dev, AT91_TWI_ACR, acr);
>> + }
>> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_CLEAR);
>
> This bit is not documented on SoCs before SAMA5D2/D4, this write
> shouldn't be done unconditionally.
>
>
Indeed, they are not present on SAMA5D4 or earlier SoCs. It is supported
on SAMA5D2 though. I will make a new version and implement the CLEAR
command only for the SoCs that support it.
Thank you for your review.
Best regards,
Codrin
Re: [PATCH v2 6/6] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode
On 24.08.2019 23:26, Michał Mirosław wrote:
> SSC driver allows only synchronous TX and RX. In slave mode for BCLK
> it uses only one of TK or RK pin, but for LRCLK it configured separate
> inputs from TF and RF pins. Allow configuration with common FS signal.
>
> Signed-off-by: Michał Mirosław
>
> ---
> v2: use alternate DT binding
> split DT and drivers/misc changes
>
> ---
> sound/soc/atmel/atmel_ssc_dai.c | 26 ++
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index 48e9eef34c0f..035d4da58f2b 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -605,14 +605,32 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream
> *substream,
> return -EINVAL;
> }
>
> - if (!atmel_ssc_cfs(ssc_p)) {
> + if (atmel_ssc_cfs(ssc_p)) {
> + /*
> + * SSC provides LRCLK
> + *
> + * Both TF and RF are generated, so use them directly.
> + */
> + rcmr |= SSC_BF(RCMR_START, fs_edge);
> + tcmr |= SSC_BF(TCMR_START, fs_edge);
Hmm, how would this work if capture and playback start/run at the same time?
> + } else {
> fslen = fslen_ext = 0;
> rcmr_period = tcmr_period = 0;
> fs_osync = SSC_FSOS_NONE;
> - }
>
> - rcmr |= SSC_BF(RCMR_START, fs_edge);
> - tcmr |= SSC_BF(TCMR_START, fs_edge);
> + if (ssc->lrclk_from_tf_pin) {
> + rcmr |= SSC_BF(RCMR_START, SSC_START_TX_RX);
> + tcmr |= SSC_BF(TCMR_START, fs_edge);
> + } else if (ssc->lrclk_from_rf_pin) {
> + /* assume RF is to be used when RK is used as BCLK
> input */
This comment is not longer true...
> + /* Note: won't work correctly on SAMA5D2 due to errata
> */
> + rcmr |= SSC_BF(RCMR_START, fs_edge);
> + tcmr |= SSC_BF(TCMR_START, SSC_START_TX_RX);
> + } else {
> + rcmr |= SSC_BF(RCMR_START, fs_edge);
> + tcmr |= SSC_BF(TCMR_START, fs_edge);
> + }
> + }
>
> if (atmel_ssc_cbs(ssc_p)) {
> /*
>
Thanks and best regards,
Codrin
Re: [PATCH v2 5/6] misc: atmel-ssc: get LRCLK pin selection from DT
On 24.08.2019 23:26, Michał Mirosław wrote:
> Store LRCLK pin selection for use by ASoC DAI driver.
>
> Signed-off-by: Michał Mirosław
Reviewed-by: Codrin Ciubotariu
Thanks and best regards,
Codrin
>
> ---
>v2: split from ASoC implementation
>
> ---
> drivers/misc/atmel-ssc.c | 9 +
> include/linux/atmel-ssc.h | 2 ++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
> index ab4144ea1f11..1322e29bc37a 100644
> --- a/drivers/misc/atmel-ssc.c
> +++ b/drivers/misc/atmel-ssc.c
> @@ -210,6 +210,15 @@ static int ssc_probe(struct platform_device *pdev)
> struct device_node *np = pdev->dev.of_node;
> ssc->clk_from_rk_pin =
> of_property_read_bool(np, "atmel,clk-from-rk-pin");
> + ssc->lrclk_from_tf_pin =
> + of_property_read_bool(np, "atmel,lrclk-from-tf-pin");
> + ssc->lrclk_from_rf_pin =
> + of_property_read_bool(np, "atmel,lrclk-from-rf-pin");
> +
> + if (ssc->lrclk_from_tf_pin && ssc->lrclk_from_rf_pin) {
> + dev_err(&pdev->dev, "both LRCLK from RK/TK options
> found in DT node");
> + return -EINVAL;
> + }
> }
>
> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> diff --git a/include/linux/atmel-ssc.h b/include/linux/atmel-ssc.h
> index 6091d2abc1eb..fbe1c2ffaa81 100644
> --- a/include/linux/atmel-ssc.h
> +++ b/include/linux/atmel-ssc.h
> @@ -21,6 +21,8 @@ struct ssc_device {
> int user;
> int irq;
> boolclk_from_rk_pin;
> + boollrclk_from_tf_pin;
> + boollrclk_from_rf_pin;
> boolsound_dai;
> };
>
>
Re: [PATCH v2 3/6] ASoC: atmel_ssc_dai: implement left-justified data mode
> Enable support for left-justified data mode for SSC-codec link.
>
> Signed-off-by: Michał Mirosław
>
> ---
> v2: rebased
I noticed you also added a description and you removed two comments from
v1. Please include all the changes in the changelog.
I already added my 'Reviewed-by' in v1, but since there are some more
changes from the previous version:
Reviewed-by: Codrin Ciubotariu
Thanks and best regards,
Codrin
>
> ---
> sound/soc/atmel/atmel_ssc_dai.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index 7dc6ec9b8c7a..48e9eef34c0f 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -564,6 +564,15 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream
> *substream,
>
> switch (ssc_p->daifmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>
> + case SND_SOC_DAIFMT_LEFT_J:
> + fs_osync = SSC_FSOS_POSITIVE;
> + fs_edge = SSC_START_RISING_RF;
> +
> + rcmr =SSC_BF(RCMR_STTDLY, 0);
> + tcmr =SSC_BF(TCMR_STTDLY, 0);
> +
> + break;
> +
> case SND_SOC_DAIFMT_I2S:
> fs_osync = SSC_FSOS_NEGATIVE;
> fs_edge = SSC_START_FALLING_RF;
>
Re: [PATCH v2 2/6] ASoC: atmel_ssc_dai: rework DAI format configuration
On 24.08.2019 23:26, Michał Mirosław wrote:
> Rework DAI format calculation in preparation for adding more formats
> later. As a side-effect this enables all CBM/CBS x CFM/CFS combinations
> for supported formats. (Note: the additional modes are not tested.)
The only mode added (and not tested) is for SND_SOC_DAIFMT_DSP_A |
SND_SOC_DAIFMT_CBM_CFS. I would mention it explicitly, instead of
something generic. Also, SND_SOC_DAIFMT_CBM_CFS is not supported for any
format, please see atmel_ssc_hw_rule_rate().
>
> Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
> interrupt is not used anyway.
>
> Signed-off-by: Michał Mirosław
>
> ---
> v2: added note about extended modes' status
> incorporated common FS (LRCLK) configuration
>
> ---
> sound/soc/atmel/atmel_ssc_dai.c | 286 +---
> 1 file changed, 80 insertions(+), 206 deletions(-)
>
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index 6f89483ac88c..7dc6ec9b8c7a 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -471,7 +471,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream
> *substream,
> int dir, channels, bits;
> u32 tfmr, rfmr, tcmr, rcmr;
> int ret;
> - int fslen, fslen_ext;
> + int fslen, fslen_ext, fs_osync, fs_edge;
> u32 cmr_div;
> u32 tcmr_period;
> u32 rcmr_period;
> @@ -558,226 +558,36 @@ static int atmel_ssc_hw_params(struct
> snd_pcm_substream *substream,
> /*
>* Compute SSC register settings.
>*/
> - switch (ssc_p->daifmt
> - & (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_MASTER_MASK)) {
>
> - case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS:
> - /*
> - * I2S format, SSC provides BCLK and LRC clocks.
> - *
> - * The SSC transmit and receive clocks are generated
> - * from the MCK divider, and the BCLK signal
> - * is output on the SSC TK line.
> - */
> -
> - if (bits > 16 && !ssc->pdata->has_fslen_ext) {
> - dev_err(dai->dev,
> - "sample size %d is too large for SSC device\n",
> - bits);
> - return -EINVAL;
> - }
> -
> - fslen_ext = (bits - 1) / 16;
> - fslen = (bits - 1) % 16;
> -
> - rcmr =SSC_BF(RCMR_PERIOD, rcmr_period)
> - | SSC_BF(RCMR_STTDLY, START_DELAY)
> - | SSC_BF(RCMR_START, SSC_START_FALLING_RF)
> - | SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> - | SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> - | SSC_BF(RCMR_CKS, SSC_CKS_DIV);
> -
> - rfmr =SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> - | SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> - | SSC_BF(RFMR_FSOS, SSC_FSOS_NEGATIVE)
> - | SSC_BF(RFMR_FSLEN, fslen)
> - | SSC_BF(RFMR_DATNB, (channels - 1))
> - | SSC_BIT(RFMR_MSBF)
> - | SSC_BF(RFMR_LOOP, 0)
> - | SSC_BF(RFMR_DATLEN, (bits - 1));
> -
> - tcmr =SSC_BF(TCMR_PERIOD, tcmr_period)
> - | SSC_BF(TCMR_STTDLY, START_DELAY)
> - | SSC_BF(TCMR_START, SSC_START_FALLING_RF)
> - | SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> - | SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS)
> - | SSC_BF(TCMR_CKS, SSC_CKS_DIV);
> -
> - tfmr =SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> - | SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> - | SSC_BF(TFMR_FSDEN, 0)
> - | SSC_BF(TFMR_FSOS, SSC_FSOS_NEGATIVE)
> - | SSC_BF(TFMR_FSLEN, fslen)
> - | SSC_BF(TFMR_DATNB, (channels - 1))
> - | SSC_BIT(TFMR_MSBF)
> - | SSC_BF(TFMR_DATDEF, 0)
> - | SSC_BF(TFMR_DATLEN, (bits - 1));
> - break;
> -
> - case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM:
> - /* I2S format, CODEC supplies BCLK and LRC clocks. */
> - rcmr =SSC_BF(RCMR_PERIOD, 0)
> - | SSC_BF(RCMR_STTDLY, START_DELAY)
> - | SSC_BF(RCMR_START, SSC_START_FALLING_RF)
> - | SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> - | SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> - | SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> -SSC_CKS_PIN : SSC_CKS_CLOCK);
> -
> - rfmr =SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> - | SSC_BF(RFMR_FSOS, SSC_FSOS_NONE)
> - | SSC_BF(RFMR_FSLEN, 0)
> - | SSC_BF(RFMR_DATNB, (channels - 1))
> -
Re: [PATCH v2 1/6] ASoC: atmel: enable SOC_SSC_PDC and SOC_SSC_DMA in Kconfig
On 24.08.2019 23:26, Michał Mirosław wrote: > Allow SSC to be used on platforms described using audio-graph-card > in Device Tree. > > Signed-off-by: Michał Mirosław Reviewed-by: Codrin Ciubotariu Thanks! Best regards, Codrin > > --- > v2: extended to PDC mode > reworked and fixed Kconfig option dependencies > > --- > sound/soc/atmel/Kconfig | 30 ++ > 1 file changed, 18 insertions(+), 12 deletions(-) > > diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig > index 06c1d5ce642c..f118c229ed82 100644 > --- a/sound/soc/atmel/Kconfig > +++ b/sound/soc/atmel/Kconfig > @@ -12,25 +12,31 @@ if SND_ATMEL_SOC > config SND_ATMEL_SOC_PDC > tristate > depends on HAS_DMA > - default m if SND_ATMEL_SOC_SSC_PDC=m && SND_ATMEL_SOC_SSC=m > - default y if SND_ATMEL_SOC_SSC_PDC=y || (SND_ATMEL_SOC_SSC_PDC=m && > SND_ATMEL_SOC_SSC=y) > - > -config SND_ATMEL_SOC_SSC_PDC > - tristate > > config SND_ATMEL_SOC_DMA > tristate > select SND_SOC_GENERIC_DMAENGINE_PCM > - default m if SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=m > - default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && > SND_ATMEL_SOC_SSC=y) > - > -config SND_ATMEL_SOC_SSC_DMA > - tristate > > config SND_ATMEL_SOC_SSC > tristate > - default y if SND_ATMEL_SOC_SSC_DMA=y || SND_ATMEL_SOC_SSC_PDC=y > - default m if SND_ATMEL_SOC_SSC_DMA=m || SND_ATMEL_SOC_SSC_PDC=m > + > +config SND_ATMEL_SOC_SSC_PDC > + tristate "SoC PCM DAI support for AT91 SSC controller using PDC" > + depends on ATMEL_SSC > + select SND_ATMEL_SOC_PDC > + select SND_ATMEL_SOC_SSC > + help > + Say Y or M if you want to add support for Atmel SSC interface > + in PDC mode configured using audio-graph-card in device-tree. > + > +config SND_ATMEL_SOC_SSC_DMA > + tristate "SoC PCM DAI support for AT91 SSC controller using DMA" > + depends on ATMEL_SSC > + select SND_ATMEL_SOC_DMA > + select SND_ATMEL_SOC_SSC > + help > + Say Y or M if you want to add support for Atmel SSC interface > + in DMA mode configured using audio-graph-card in device-tree. > > config SND_AT91_SOC_SAM9G20_WM8731 > tristate "SoC Audio support for WM8731-based At91sam9g20 evaluation > board" >
Re: [alsa-devel] [PATCH 1/2] ASoC: codecs: ad193x: Group register initialization at probe
On 03.07.2019 10:39, Tzung-Bi Shih wrote:
> On Thu, Jun 27, 2019 at 8:05 PM Codrin Ciubotariu
> wrote:
>> +struct ad193x_reg_default {
>> + unsigned int reg;
>> + unsigned int val;
>> +};
> You probably don't need to define this. There is a struct
> reg_sequence in regmap.h.
>
>> +
>> +/* codec register values to set after reset */
>> +static void ad193x_reg_default_init(struct ad193x_priv *ad193x)
>> +{
>> + const struct ad193x_reg_default reg_init[] = {
>> + { 0, 0x99 }, /* PLL_CLK_CTRL0: pll input: mclki/xi
>> 12.288Mhz */
>> + { 1, 0x04 }, /* PLL_CLK_CTRL1: no on-chip Vref */
>> + { 2, 0x40 }, /* DAC_CTRL0: TDM mode */
>> + { 4, 0x1A }, /* DAC_CTRL2: 48kHz de-emphasis, unmute dac
>> */
>> + { 5, 0x00 }, /* DAC_CHNL_MUTE: unmute DAC channels */
>> + };
>> + const struct ad193x_reg_default reg_adc_init[] = {
>> + { 14, 0x03 }, /* ADC_CTRL0: high-pass filter enable */
>> + { 15, 0x43 }, /* ADC_CTRL1: sata delay=1, adc aux mode */
>> + };
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(reg_init); i++)
>> + regmap_write(ad193x->regmap, reg_init[i].reg,
>> reg_init[i].val);
>> +
>> + if (ad193x_has_adc(ad193x)) {
>> + for (i = 0; i < ARRAY_SIZE(reg_adc_init); i++) {
>> + regmap_write(ad193x->regmap, reg_adc_init[i].reg,
>> +reg_adc_init[i].val);
>> + }
>> + }
> And you could use regmap_multi_reg_write( ) to substitute the two for-loops.
>
> See
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-June/151090.html
> as an example. It also has some reg initializations in component
> probe( ).
>
Your solution is certainly more elegant. I will make a patch and switch
to regmap_multi_reg_write().
Thank you for your review.
Best regards,
Codrin
Re: [PATCH 2/2] ASoC: codecs: ad193x: Reset DAC Control 1 register at probe
On 26.06.2019 14:23, Mark Brown wrote: > On Wed, Jun 26, 2019 at 01:49:47PM +0300, Codrin Ciubotariu wrote: >> Since the ad193x codecs have no software reset, we have to reinitialize the >> registers after a hardware reset. For example, if we change the >> device-tree between these resets, changing the audio format of the DAI link >> from DSP_A with 8 TDM channels to I2S 2 channels, DAC Control 1 register >> will remain configured for 8 channels. This patch resets this register at >> probe to its default value. > > Would it not be more robust/complete to have a set of register defaults > and write the whole lot out rather than individually going through and > adding writes for specific registers as needed? > It would indeed. I will make two patches, one that implements what you suggested, for the registers that we touch only, and another one that will add AD193X_DAC_CTRL1 to these defaults. You can drop this patch. Thanks and best regards, Codrin
[PATCH] clk: at91: generated: Truncate divisor to GENERATED_MAX_DIV + 1
From: Codrin Ciubotariu
In clk_generated_determine_rate(), if the divisor is greater than
GENERATED_MAX_DIV + 1, then the wrong best_rate will be returned.
If clk_generated_set_rate() will be called later with this wrong
rate, it will return -EINVAL, so the generated clock won't change
its value. Do no let the divisor be greater than GENERATED_MAX_DIV + 1.
Fixes: 8c7aa6328947 ("clk: at91: clk-generated: remove useless divisor loop")
Signed-off-by: Codrin Ciubotariu
---
drivers/clk/at91/clk-generated.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/clk/at91/clk-generated.c b/drivers/clk/at91/clk-generated.c
index 5f18847965c1..290cffe35deb 100644
--- a/drivers/clk/at91/clk-generated.c
+++ b/drivers/clk/at91/clk-generated.c
@@ -146,6 +146,8 @@ static int clk_generated_determine_rate(struct clk_hw *hw,
continue;
div = DIV_ROUND_CLOSEST(parent_rate, req->rate);
+ if (div > GENERATED_MAX_DIV + 1)
+ div = GENERATED_MAX_DIV + 1;
clk_generated_best_diff(req, parent, parent_rate, div,
&best_diff, &best_rate);
--
2.20.1
[PATCH 2/2] ASoC: mchp-i2s-mcc: add driver for I2SC Multi-Channel Controller
From: Codrin Ciubotariu The Inter-IC Sound Controller (I2SMCC) provides a 5-wire, bidirectional, synchronous, digital audio link to external audio devices: I2SMCC_DIN, I2SMCC_DOUT, I2SMCC_WS, I2SMCC_CK, and I2SMCC_MCK pins. The I2SMCC complies with the Inter-IC Sound (I2S) bus specification and supports a Time Division Multiplexed (TDM) interface with external multi-channel audio codecs. The I2SMCC consists of a receiver, a transmitter and a common clock generator that can be enabled separately to provide Master, Slave or Controller modes with receiver and/or transmitter active. DMA Controller channels, separate for the receiver and for the transmitter, allow a continuous high bit rate data transfer without processor intervention to the following: - Audio CODECs in Master, Slave, or Controller mode - Stereo DAC or ADC through a dedicated I2S serial interface - Multi-channel or multiple stereo DACs or ADCs, using the TDM format This IP is embedded in Microchip's new sam9x60 SoC. Signed-off-by: Codrin Ciubotariu --- sound/soc/atmel/Kconfig| 14 + sound/soc/atmel/Makefile | 2 + sound/soc/atmel/mchp-i2s-mcc.c | 974 + 3 files changed, 990 insertions(+) create mode 100644 sound/soc/atmel/mchp-i2s-mcc.c diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig index 64f86f0b87e5..c473b9e463ab 100644 --- a/sound/soc/atmel/Kconfig +++ b/sound/soc/atmel/Kconfig @@ -109,4 +109,18 @@ config SND_SOC_MIKROE_PROTO using I2C over SDA (MPU Data Input) and SCL (MPU Clock Input) pins. Both playback and capture are supported. +config SND_MCHP_SOC_I2S_MCC + tristate "Microchip ASoC driver for boards using I2S MCC" + depends on OF && (ARCH_AT91 || COMPILE_TEST) + select SND_SOC_GENERIC_DMAENGINE_PCM + select REGMAP_MMIO + help + Say Y or M if you want to add support for I2S Multi-Channel ASoC + driver on the following Microchip platforms: + - sam9x60 + + The I2SMCC complies with the Inter-IC Sound (I2S) bus specification + and supports a Time Division Multiplexed (TDM) interface with + external multi-channel audio codecs. + endif diff --git a/sound/soc/atmel/Makefile b/sound/soc/atmel/Makefile index 9f41bfa0fea3..1f6890ed3738 100644 --- a/sound/soc/atmel/Makefile +++ b/sound/soc/atmel/Makefile @@ -4,11 +4,13 @@ snd-soc-atmel-pcm-pdc-objs := atmel-pcm-pdc.o snd-soc-atmel-pcm-dma-objs := atmel-pcm-dma.o snd-soc-atmel_ssc_dai-objs := atmel_ssc_dai.o snd-soc-atmel-i2s-objs := atmel-i2s.o +snd-soc-mchp-i2s-mcc-objs := mchp-i2s-mcc.o obj-$(CONFIG_SND_ATMEL_SOC_PDC) += snd-soc-atmel-pcm-pdc.o obj-$(CONFIG_SND_ATMEL_SOC_DMA) += snd-soc-atmel-pcm-dma.o obj-$(CONFIG_SND_ATMEL_SOC_SSC) += snd-soc-atmel_ssc_dai.o obj-$(CONFIG_SND_ATMEL_SOC_I2S) += snd-soc-atmel-i2s.o +obj-$(CONFIG_SND_MCHP_SOC_I2S_MCC) += snd-soc-mchp-i2s-mcc.o # AT91 Machine Support snd-soc-sam9g20-wm8731-objs := sam9g20_wm8731.o diff --git a/sound/soc/atmel/mchp-i2s-mcc.c b/sound/soc/atmel/mchp-i2s-mcc.c new file mode 100644 index ..86495883ca3f --- /dev/null +++ b/sound/soc/atmel/mchp-i2s-mcc.c @@ -0,0 +1,974 @@ +// SPDX-License-Identifier: GPL-2.0 +// +// Driver for Microchip I2S Multi-channel controller +// +// Copyright (C) 2018 Microchip Technology Inc. and its subsidiaries +// +// Author: Codrin Ciubotariu + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +/* + * I2S Controller Register map + */ +#define MCHP_I2SMCC_CR 0x /* Control Register */ +#define MCHP_I2SMCC_MRA0x0004 /* Mode Register A */ +#define MCHP_I2SMCC_MRB0x0008 /* Mode Register B */ +#define MCHP_I2SMCC_SR 0x000C /* Status Register */ +#define MCHP_I2SMCC_IERA 0x0010 /* Interrupt Enable Register A */ +#define MCHP_I2SMCC_IDRA 0x0014 /* Interrupt Disable Register A */ +#define MCHP_I2SMCC_IMRA 0x0018 /* Interrupt Mask Register A */ +#define MCHP_I2SMCC_ISRA 0X001C /* Interrupt Status Register A */ + +#define MCHP_I2SMCC_IERB 0x0020 /* Interrupt Enable Register B */ +#define MCHP_I2SMCC_IDRB 0x0024 /* Interrupt Disable Register B */ +#define MCHP_I2SMCC_IMRB 0x0028 /* Interrupt Mask Register B */ +#define MCHP_I2SMCC_ISRB 0X002C /* Interrupt Status Register B */ + +#define MCHP_I2SMCC_RHR0x0030 /* Receiver Holding Register */ +#define MCHP_I2SMCC_THR0x0034 /* Transmitter Holding Register */ + +#define MCHP_I2SMCC_RHL0R 0x0040 /* Receiver Holding Left 0 Register */ +#define MCHP_I2SMCC_RHR0R 0x0044 /* Receiver Holding Right 0 Register */ + +#define MCHP_I2SMCC_RHL1R 0x0048 /* Receiver Holding Left 1 Register */ +#define MCHP_I2SMCC_RHR1R 0x004C /* Receiver Holding Right 1 Register */ + +#define MCHP_I2SMCC_RHL2R
[PATCH 1/2] ASoC: mchp-i2s-mcc: dt-bindings: add DT bindings for I2S Multi-Channel Controller
From: Codrin Ciubotariu
This patch adds DT bindings for the new Microchip I2S Multi-Channel
controller embedded inside sam9x60 SoCs.
Signed-off-by: Codrin Ciubotariu
---
.../bindings/sound/mchp-i2s-mcc.txt | 43 +++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/mchp-i2s-mcc.txt
diff --git a/Documentation/devicetree/bindings/sound/mchp-i2s-mcc.txt
b/Documentation/devicetree/bindings/sound/mchp-i2s-mcc.txt
new file mode 100644
index ..91ec83a6faed
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/mchp-i2s-mcc.txt
@@ -0,0 +1,43 @@
+* Microchip I2S Multi-Channel Controller
+
+Required properties:
+- compatible: Should be "microchip,sam9x60-i2smcc".
+- reg:Should be the physical base address of the controller and the
+ length of memory mapped region.
+- interrupts: Should contain the interrupt for the controller.
+- dmas: Should be one per channel name listed in the dma-names
property,
+ as described in atmel-dma.txt and dma.txt files.
+- dma-names: Identifier string for each DMA request line in the dmas
property.
+ Two dmas have to be defined, "tx" and "rx".
+- clocks: Must contain an entry for each entry in clock-names.
+ Please refer to clock-bindings.txt.
+- clock-names:Should be one of each entry matching the clocks phandles
list:
+ - "pclk" (peripheral clock) Required.
+ - "gclk" (generated clock) Optional (1).
+
+Optional properties:
+- pinctrl-0: Should specify pin control groups used for this controller.
+- princtrl-names: Should contain only one value - "default".
+
+
+(1) : Only the peripheral clock is required. The generated clock is optional
+ and should be set mostly when Master Mode is required.
+
+Example:
+
+ i2s@f001c000 {
+ compatible = "microchip,sam9x60-i2smcc";
+ reg = <0xf001c000 0x100>;
+ interrupts = <34 IRQ_TYPE_LEVEL_HIGH 7>;
+ dmas = <&dma0
+ (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) |
+AT91_XDMAC_DT_PERID(36))>,
+ <&dma0
+ (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1) |
+AT91_XDMAC_DT_PERID(37))>;
+ dma-names = "tx", "rx";
+ clocks = <&i2s_clk>, <&i2s_gclk>;
+ clock-names = "pclk", "gclk";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_i2s_default>;
+ };
--
2.17.1
Re: [PATCH v2 2/2] ASoC: codecs: pcm186x: Fix energysense SLEEP bit
On 19.02.2019 19:06, Andrew F. Davis wrote:
> On 2/19/19 10:29 AM, codrin.ciubota...@microchip.com wrote:
>> From: Codrin Ciubotariu
>>
>> The ADCs are sleeping when the SLEEP bit is set and running when it's
>> cleared, so the bit should be inverted.
>> Tested on pcm1863.
>>
>
> Did this work for you before? Strange it would if reversed, I wonder if
> the SLEEP bit is really doing anything here. Can investigate later, for
> this patch:
The SLEEP bit seems to work. At least STATE from register 114 says the
IP is sleeping. Without this change, if pcm1863 is used as master, I can
see BCK and LRCK generated while no sound is played. Once I start an
audio stream, the clocks stop.
Thank you for your ACK. I would really appreciate if you could also test
it on your side.
Best regards,
Codrin
>
> Acked-by: Andrew F. Davis
>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>>
>> Changes in v2:
>> - none;
>>
>> sound/soc/codecs/pcm186x.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
>> index c36a391fec8a..c5fcc632f670 100644
>> --- a/sound/soc/codecs/pcm186x.c
>> +++ b/sound/soc/codecs/pcm186x.c
>> @@ -158,7 +158,7 @@ static const struct snd_soc_dapm_widget
>> pcm1863_dapm_widgets[] = {
>> * Put the codec into SLEEP mode when not in use, allowing the
>> * Energysense mechanism to operate.
>> */
>> -SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 0),
>> +SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 1),
>> };
>>
>> static const struct snd_soc_dapm_widget pcm1865_dapm_widgets[] = {
>> @@ -184,8 +184,8 @@ static const struct snd_soc_dapm_widget
>> pcm1865_dapm_widgets[] = {
>> * Put the codec into SLEEP mode when not in use, allowing the
>> * Energysense mechanism to operate.
>> */
>> -SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 0),
>> -SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 0),
>> +SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 1),
>> +SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 1),
>> };
>>
>> static const struct snd_soc_dapm_route pcm1863_dapm_routes[] = {
>>
[PATCH v2 2/2] ASoC: codecs: pcm186x: Fix energysense SLEEP bit
From: Codrin Ciubotariu
The ADCs are sleeping when the SLEEP bit is set and running when it's
cleared, so the bit should be inverted.
Tested on pcm1863.
Signed-off-by: Codrin Ciubotariu
---
Changes in v2:
- none;
sound/soc/codecs/pcm186x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
index c36a391fec8a..c5fcc632f670 100644
--- a/sound/soc/codecs/pcm186x.c
+++ b/sound/soc/codecs/pcm186x.c
@@ -158,7 +158,7 @@ static const struct snd_soc_dapm_widget
pcm1863_dapm_widgets[] = {
* Put the codec into SLEEP mode when not in use, allowing the
* Energysense mechanism to operate.
*/
- SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 0),
+ SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 1),
};
static const struct snd_soc_dapm_widget pcm1865_dapm_widgets[] = {
@@ -184,8 +184,8 @@ static const struct snd_soc_dapm_widget
pcm1865_dapm_widgets[] = {
* Put the codec into SLEEP mode when not in use, allowing the
* Energysense mechanism to operate.
*/
- SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 0),
- SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 0),
+ SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 1),
+ SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 1),
};
static const struct snd_soc_dapm_route pcm1863_dapm_routes[] = {
--
2.17.1
[PATCH v2 1/2] ASoC: codecs: pcm186x: fix wrong usage of DECLARE_TLV_DB_SCALE()
From: Codrin Ciubotariu
According to DS, the gain is between -12 dB and 40 dB, with a 0.5 dB step.
Tested on pcm1863.
Signed-off-by: Codrin Ciubotariu
---
Changes in v2:
- fixed title - added correct macro;
- revert the removal of a new line;
sound/soc/codecs/pcm186x.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
index 809b7e9f03ca..c36a391fec8a 100644
--- a/sound/soc/codecs/pcm186x.c
+++ b/sound/soc/codecs/pcm186x.c
@@ -42,7 +42,7 @@ struct pcm186x_priv {
bool is_master_mode;
};
-static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 4000, 50);
+static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 50, 0);
static const struct snd_kcontrol_new pcm1863_snd_controls[] = {
SOC_DOUBLE_R_S_TLV("ADC Capture Volume", PCM186X_PGA_VAL_CH1_L,
--
2.17.1
[PATCH 2/2] ASoC: codecs: pcm186x: Fix energysense SLEEP bit
From: Codrin Ciubotariu
The ADCs are sleeping when the SLEEP bit is set and running when it's
cleared, so the bit should be inverted.
Tested on pcm1863.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/pcm186x.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
index 7947279243f9..4477d678797f 100644
--- a/sound/soc/codecs/pcm186x.c
+++ b/sound/soc/codecs/pcm186x.c
@@ -157,7 +157,7 @@ static const struct snd_soc_dapm_widget
pcm1863_dapm_widgets[] = {
* Put the codec into SLEEP mode when not in use, allowing the
* Energysense mechanism to operate.
*/
- SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 0),
+ SND_SOC_DAPM_ADC("ADC", "HiFi Capture", PCM186X_POWER_CTRL, 1, 1),
};
static const struct snd_soc_dapm_widget pcm1865_dapm_widgets[] = {
@@ -183,8 +183,8 @@ static const struct snd_soc_dapm_widget
pcm1865_dapm_widgets[] = {
* Put the codec into SLEEP mode when not in use, allowing the
* Energysense mechanism to operate.
*/
- SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 0),
- SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 0),
+ SND_SOC_DAPM_ADC("ADC1", "HiFi Capture 1", PCM186X_POWER_CTRL, 1, 1),
+ SND_SOC_DAPM_ADC("ADC2", "HiFi Capture 2", PCM186X_POWER_CTRL, 1, 1),
};
static const struct snd_soc_dapm_route pcm1863_dapm_routes[] = {
--
2.17.1
[PATCH 1/2] ASoC: codecs: pcm186x: fix wrong usage of DECLARE_TLV_DB_LINEAR()
From: Codrin Ciubotariu
According to DS, the gain is between -12 dB and 40 dB, with a 0.5 dB step.
Tested on pcm1863.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/pcm186x.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
index 809b7e9f03ca..7947279243f9 100644
--- a/sound/soc/codecs/pcm186x.c
+++ b/sound/soc/codecs/pcm186x.c
@@ -6,7 +6,6 @@
* Andreas Dannenberg
* Andrew F. Davis
*/
-
#include
#include
#include
@@ -42,7 +41,7 @@ struct pcm186x_priv {
bool is_master_mode;
};
-static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 4000, 50);
+static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 50, 0);
static const struct snd_kcontrol_new pcm1863_snd_controls[] = {
SOC_DOUBLE_R_S_TLV("ADC Capture Volume", PCM186X_PGA_VAL_CH1_L,
--
2.17.1
Re: [PATCH 1/2] ASoC: codecs: pcm186x: fix wrong usage of DECLARE_TLV_DB_LINEAR()
On 19.02.2019 18:15, Codrin Ciubotariu - M19940 wrote:
> From: Codrin Ciubotariu
>
> According to DS, the gain is between -12 dB and 40 dB, with a 0.5 dB step.
> Tested on pcm1863.
>
> Signed-off-by: Codrin Ciubotariu
> ---
> sound/soc/codecs/pcm186x.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sound/soc/codecs/pcm186x.c b/sound/soc/codecs/pcm186x.c
> index 809b7e9f03ca..7947279243f9 100644
> --- a/sound/soc/codecs/pcm186x.c
> +++ b/sound/soc/codecs/pcm186x.c
> @@ -6,7 +6,6 @@
>* Andreas Dannenberg
>* Andrew F. Davis
>*/
> -
> #include
> #include
> #include
> @@ -42,7 +41,7 @@ struct pcm186x_priv {
> bool is_master_mode;
> };
>
> -static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 4000, 50);
> +static const DECLARE_TLV_DB_SCALE(pcm186x_pga_tlv, -1200, 50, 0);
>
> static const struct snd_kcontrol_new pcm1863_snd_controls[] = {
> SOC_DOUBLE_R_S_TLV("ADC Capture Volume", PCM186X_PGA_VAL_CH1_L,
>
the title is wrong, there is no DECLARE_TLV_DB_LINEAR()present. Will
send v2.
Best regards,
Codrin
Re: [PATCH 0/5] ASoC: codecs: ad193x: Several fixes and imprevements
On 18.02.2019 18:10, Codrin Ciubotariu - M19940 wrote: > From: Codrin Ciubotariu > > This patch set contains some changes I needed to make I2S and TDM > (DSP_A) formats work. I tested only with ad1936, but the patches actually, it's ad1934. Sorry about that. Best regards, Codrin > should work fine for all the codecs that use this driver. Although I > checked the DS for all of them (I hope) to assure that the patches > apply, I would really appreciate if someone else can test them on > another codec. > > Codrin Ciubotariu (5): >ASoC: codecs: ad193x: Remove capture support for codecs without ADC >ASoC: codecs: ad193x: Set constraint to always have 32 sample bits >ASoC: codecs: ad193x: Fix frame polarity for DSP_A format >ASoC: codecs: ad193x: Add runtime support for DSP_A and I2S modes >ASoC: codecs: ad193x: Add support to disable on-chip PLL > > sound/soc/codecs/ad193x.c | 76 --- > sound/soc/codecs/ad193x.h | 8 + > 2 files changed, 80 insertions(+), 4 deletions(-) >
[PATCH 4/5] ASoC: codecs: ad193x: Add runtime support for DSP_A and I2S modes
From: Codrin Ciubotariu
The driver only supports DPS_A for DAC, which is configured at probe.
This patch adds support for DSP_A and I2S modes by using the set_fmt()
callback.
A trivial break is also removed from a case's default branch.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/ad193x.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/ad193x.c b/sound/soc/codecs/ad193x.c
index 315ec9775118..f8cf182518a3 100644
--- a/sound/soc/codecs/ad193x.c
+++ b/sound/soc/codecs/ad193x.c
@@ -188,23 +188,26 @@ static int ad193x_set_dai_fmt(struct snd_soc_dai
*codec_dai,
{
struct ad193x_priv *ad193x =
snd_soc_component_get_drvdata(codec_dai->component);
unsigned int adc_serfmt = 0;
+ unsigned int dac_serfmt = 0;
unsigned int adc_fmt = 0;
unsigned int dac_fmt = 0;
/* At present, the driver only support AUX ADC mode(SND_SOC_DAIFMT_I2S
-* with TDM) and ADC&DAC TDM mode(SND_SOC_DAIFMT_DSP_A)
+* with TDM), ADC&DAC TDM mode(SND_SOC_DAIFMT_DSP_A) and DAC I2S mode
+* (SND_SOC_DAIFMT_I2S)
*/
switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
case SND_SOC_DAIFMT_I2S:
adc_serfmt |= AD193X_ADC_SERFMT_TDM;
+ dac_serfmt |= AD193X_DAC_SERFMT_STEREO;
break;
case SND_SOC_DAIFMT_DSP_A:
adc_serfmt |= AD193X_ADC_SERFMT_AUX;
+ dac_serfmt |= AD193X_DAC_SERFMT_TDM;
break;
default:
if (ad193x_has_adc(ad193x))
return -EINVAL;
- break;
}
switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
@@ -261,6 +264,8 @@ static int ad193x_set_dai_fmt(struct snd_soc_dai *codec_dai,
regmap_update_bits(ad193x->regmap, AD193X_ADC_CTRL2,
AD193X_ADC_FMT_MASK, adc_fmt);
}
+ regmap_update_bits(ad193x->regmap, AD193X_DAC_CTRL0,
+ AD193X_DAC_SERFMT_MASK, dac_serfmt);
regmap_update_bits(ad193x->regmap, AD193X_DAC_CTRL1,
AD193X_DAC_FMT_MASK, dac_fmt);
--
2.17.1
[PATCH 0/5] ASoC: codecs: ad193x: Several fixes and imprevements
From: Codrin Ciubotariu This patch set contains some changes I needed to make I2S and TDM (DSP_A) formats work. I tested only with ad1936, but the patches should work fine for all the codecs that use this driver. Although I checked the DS for all of them (I hope) to assure that the patches apply, I would really appreciate if someone else can test them on another codec. Codrin Ciubotariu (5): ASoC: codecs: ad193x: Remove capture support for codecs without ADC ASoC: codecs: ad193x: Set constraint to always have 32 sample bits ASoC: codecs: ad193x: Fix frame polarity for DSP_A format ASoC: codecs: ad193x: Add runtime support for DSP_A and I2S modes ASoC: codecs: ad193x: Add support to disable on-chip PLL sound/soc/codecs/ad193x.c | 76 --- sound/soc/codecs/ad193x.h | 8 + 2 files changed, 80 insertions(+), 4 deletions(-) -- 2.17.1
[PATCH 2/5] ASoC: codecs: ad193x: Set constraint to always have 32 sample bits
From: Codrin Ciubotariu
DACs and ADCs on ad193x codecs require a 32 bit slot size. We should
assure that no other size is used.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/ad193x.c | 16
1 file changed, 16 insertions(+)
diff --git a/sound/soc/codecs/ad193x.c b/sound/soc/codecs/ad193x.c
index 21a38cc9e3da..c16c9969d1a0 100644
--- a/sound/soc/codecs/ad193x.c
+++ b/sound/soc/codecs/ad193x.c
@@ -37,6 +37,13 @@ static SOC_ENUM_SINGLE_DECL(ad193x_deemp_enum,
AD193X_DAC_CTRL2, 1,
static const DECLARE_TLV_DB_MINMAX(adau193x_tlv, -9563, 0);
+static const unsigned int ad193x_sb[] = {32};
+
+static struct snd_pcm_hw_constraint_list constr = {
+ .list = ad193x_sb,
+ .count = ARRAY_SIZE(ad193x_sb),
+};
+
static const struct snd_kcontrol_new ad193x_snd_controls[] = {
/* DAC volume control */
SOC_DOUBLE_R_TLV("DAC1 Volume", AD193X_DAC_L1_VOL,
@@ -321,7 +328,16 @@ static int ad193x_hw_params(struct snd_pcm_substream
*substream,
return 0;
}
+static int ad193x_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ return snd_pcm_hw_constraint_list(substream->runtime, 0,
+ SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
+ &constr);
+}
+
static const struct snd_soc_dai_ops ad193x_dai_ops = {
+ .startup = ad193x_startup,
.hw_params = ad193x_hw_params,
.digital_mute = ad193x_mute,
.set_tdm_slot = ad193x_set_tdm_slot,
--
2.17.1
[PATCH 1/5] ASoC: codecs: ad193x: Remove capture support for codecs without ADC
From: Codrin Ciubotariu
Some ad193x codecs don't have ADCs, so they have no capture capabilities.
This way, we can use this driver in multicodec cards.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/ad193x.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/ad193x.c b/sound/soc/codecs/ad193x.c
index 4b60ebee491d..21a38cc9e3da 100644
--- a/sound/soc/codecs/ad193x.c
+++ b/sound/soc/codecs/ad193x.c
@@ -351,6 +351,20 @@ static struct snd_soc_dai_driver ad193x_dai = {
.ops = &ad193x_dai_ops,
};
+/* codec DAI instance for DAC only */
+static struct snd_soc_dai_driver ad193x_no_adc_dai = {
+ .name = "ad193x-hifi",
+ .playback = {
+ .stream_name = "Playback",
+ .channels_min = 2,
+ .channels_max = 8,
+ .rates = SNDRV_PCM_RATE_48000,
+ .formats = SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S16_LE |
+ SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S24_LE,
+ },
+ .ops = &ad193x_dai_ops,
+};
+
static int ad193x_component_probe(struct snd_soc_component *component)
{
struct ad193x_priv *ad193x = snd_soc_component_get_drvdata(component);
@@ -444,8 +458,11 @@ int ad193x_probe(struct device *dev, struct regmap *regmap,
dev_set_drvdata(dev, ad193x);
+ if (ad193x_has_adc(ad193x))
+ return devm_snd_soc_register_component(dev,
&soc_component_dev_ad193x,
+ &ad193x_dai, 1);
return devm_snd_soc_register_component(dev, &soc_component_dev_ad193x,
- &ad193x_dai, 1);
+ &ad193x_no_adc_dai, 1);
}
EXPORT_SYMBOL_GPL(ad193x_probe);
--
2.17.1
[PATCH 3/5] ASoC: codecs: ad193x: Fix frame polarity for DSP_A format
From: Codrin Ciubotariu
By default, the codec starts to interpret the left (first) channel on
the falling edge (low polarity) of LRCLK. However, for DSP_A, the left
channel needs to start on the rising edge of LRCLK. This patch fixes
this channel swap by toggling the bit which selects the LRCLK polarity.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/ad193x.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/sound/soc/codecs/ad193x.c b/sound/soc/codecs/ad193x.c
index c16c9969d1a0..315ec9775118 100644
--- a/sound/soc/codecs/ad193x.c
+++ b/sound/soc/codecs/ad193x.c
@@ -228,6 +228,12 @@ static int ad193x_set_dai_fmt(struct snd_soc_dai
*codec_dai,
return -EINVAL;
}
+ /* For DSP_*, LRCLK's polarity must be inverted */
+ if (fmt & SND_SOC_DAIFMT_DSP_A) {
+ change_bit(ffs(AD193X_DAC_LEFT_HIGH) - 1,
+ (unsigned long *)&dac_fmt);
+ }
+
switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
case SND_SOC_DAIFMT_CBM_CFM: /* codec clk & frm master */
adc_fmt |= AD193X_ADC_LCR_MASTER;
--
2.17.1
[PATCH 5/5] ASoC: codecs: ad193x: Add support to disable on-chip PLL
From: Codrin Ciubotariu
The on-chip PLL can be disabled if on the MCLKI pin we have an external
clock at 512 x fs. This clock can be used as direct internal clock for
ADCs or DACs.
To support this, we add an extra clock id that can be configured
using the set_sysclk() callback.
Signed-off-by: Codrin Ciubotariu
---
sound/soc/codecs/ad193x.c | 26 +-
sound/soc/codecs/ad193x.h | 8
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/sound/soc/codecs/ad193x.c b/sound/soc/codecs/ad193x.c
index f8cf182518a3..96d7cb2e4a56 100644
--- a/sound/soc/codecs/ad193x.c
+++ b/sound/soc/codecs/ad193x.c
@@ -100,6 +100,15 @@ static const struct snd_soc_dapm_widget
ad193x_adc_widgets[] = {
SND_SOC_DAPM_INPUT("ADC2IN"),
};
+static int ad193x_check_pll(struct snd_soc_dapm_widget *source,
+ struct snd_soc_dapm_widget *sink)
+{
+ struct snd_soc_component *component =
snd_soc_dapm_to_component(source->dapm);
+ struct ad193x_priv *ad193x = snd_soc_component_get_drvdata(component);
+
+ return !!ad193x->sysclk;
+}
+
static const struct snd_soc_dapm_route audio_paths[] = {
{ "DAC", NULL, "SYSCLK" },
{ "DAC Output", NULL, "DAC" },
@@ -108,7 +117,7 @@ static const struct snd_soc_dapm_route audio_paths[] = {
{ "DAC2OUT", NULL, "DAC Output" },
{ "DAC3OUT", NULL, "DAC Output" },
{ "DAC4OUT", NULL, "DAC Output" },
- { "SYSCLK", NULL, "PLL_PWR" },
+ { "SYSCLK", NULL, "PLL_PWR", &ad193x_check_pll },
};
static const struct snd_soc_dapm_route ad193x_adc_audio_paths[] = {
@@ -276,7 +285,22 @@ static int ad193x_set_dai_sysclk(struct snd_soc_dai
*codec_dai,
int clk_id, unsigned int freq, int dir)
{
struct snd_soc_component *component = codec_dai->component;
+ struct snd_soc_dapm_context *dapm =
snd_soc_component_get_dapm(component);
struct ad193x_priv *ad193x = snd_soc_component_get_drvdata(component);
+
+ if (clk_id == AD193X_SYSCLK_MCLK) {
+ /* MCLK must be 512 x fs */
+ if (dir == SND_SOC_CLOCK_OUT || freq != 24576000)
+ return -EINVAL;
+
+ regmap_update_bits(ad193x->regmap, AD193X_PLL_CLK_CTRL1,
+ AD193X_PLL_SRC_MASK,
+ AD193X_PLL_DAC_SRC_MCLK |
+ AD193X_PLL_CLK_SRC_MCLK);
+
+ snd_soc_dapm_sync(dapm);
+ return 0;
+ }
switch (freq) {
case 12288000:
case 18432000:
diff --git a/sound/soc/codecs/ad193x.h b/sound/soc/codecs/ad193x.h
index 8b1e65f928d2..27d6afbd7dfb 100644
--- a/sound/soc/codecs/ad193x.h
+++ b/sound/soc/codecs/ad193x.h
@@ -31,6 +31,11 @@ int ad193x_probe(struct device *dev, struct regmap *regmap,
#define AD193X_PLL_INPUT_512(2 << 1)
#define AD193X_PLL_INPUT_768(3 << 1)
#define AD193X_PLL_CLK_CTRL10x01
+#define AD193X_PLL_SRC_MASK0x03
+#define AD193X_PLL_DAC_SRC_PLL 0
+#define AD193X_PLL_DAC_SRC_MCLK 1
+#define AD193X_PLL_CLK_SRC_PLL (0 << 1)
+#define AD193X_PLL_CLK_SRC_MCLK(1 << 1)
#define AD193X_DAC_CTRL00x02
#define AD193X_DAC_POWERDOWN 0x01
#define AD193X_DAC_SERFMT_MASK 0xC0
@@ -96,4 +101,7 @@ int ad193x_probe(struct device *dev, struct regmap *regmap,
#define AD193X_NUM_REGS 17
+#define AD193X_SYSCLK_PLL 0
+#define AD193X_SYSCLK_MCLK 1
+
#endif
--
2.17.1
Re: [PATCH v2] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
On 23.01.2019 18:33, Codrin Ciubotariu - M19940 wrote:
> From: Codrin Ciubotariu
>
> atchan->status variable is used to store two different information:
> - pass channel interrupts status from interrupt handler to tasklet;
> - channel information like whether it is cyclic or paused;
>
> This causes a bug when device_terminate_all() is called,
> (AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
> of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
> atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
> a new descriptor for a cyclic transfer is created, the driver reports
> the channel as in use:
>
> if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
> dev_err(chan2dev(chan), "channel currently used\n");
> return NULL;
> }
>
> This patch fixes the bug by adding a different struct member to keep
> the interrupts status separated from the channel status bits.
>
> Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA
> Controller driver")
> Signed-off-by: Codrin Ciubotariu
> Acked-by: Ludovic Desroches
> ---
Forgot to add:
Changes in v2:
- changed commit message;
If you need v3 for this let me know.
Best regards,
Codrin
[PATCH v2] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Codrin Ciubotariu
atchan->status variable is used to store two different information:
- pass channel interrupts status from interrupt handler to tasklet;
- channel information like whether it is cyclic or paused;
This causes a bug when device_terminate_all() is called,
(AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
a new descriptor for a cyclic transfer is created, the driver reports
the channel as in use:
if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
dev_err(chan2dev(chan), "channel currently used\n");
return NULL;
}
This patch fixes the bug by adding a different struct member to keep
the interrupts status separated from the channel status bits.
Fixes: e1f7c9eee707 ("dmaengine: at_xdmac: creation of the atmel eXtended DMA
Controller driver")
Signed-off-by: Codrin Ciubotariu
Acked-by: Ludovic Desroches
---
drivers/dma/at_xdmac.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 4e557684f792..fe69dccfa0c0 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -203,6 +203,7 @@ struct at_xdmac_chan {
u32 save_cim;
u32 save_cnda;
u32 save_cndc;
+ u32 irq_status;
unsigned long status;
struct tasklet_struct tasklet;
struct dma_slave_config sconfig;
@@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
struct at_xdmac_desc*desc;
u32 error_mask;
- dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
-__func__, atchan->status);
+ dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
+ __func__, atchan->irq_status);
error_mask = AT_XDMAC_CIS_RBEIS
| AT_XDMAC_CIS_WBEIS
@@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
if (at_xdmac_chan_is_cyclic(atchan)) {
at_xdmac_handle_cyclic(atchan);
- } else if ((atchan->status & AT_XDMAC_CIS_LIS)
- || (atchan->status & error_mask)) {
+ } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
+ || (atchan->irq_status & error_mask)) {
struct dma_async_tx_descriptor *txd;
- if (atchan->status & AT_XDMAC_CIS_RBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
dev_err(chan2dev(&atchan->chan), "read bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_WBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
dev_err(chan2dev(&atchan->chan), "write bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_ROIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
dev_err(chan2dev(&atchan->chan), "request overflow
error!!!");
spin_lock(&atchan->lock);
@@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
*dev_id)
atchan = &atxdmac->chan[i];
chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
- atchan->status = chan_status & chan_imr;
+ atchan->irq_status = chan_status & chan_imr;
dev_vdbg(atxdmac->dma.dev,
"%s: chan%d: imr=0x%x, status=0x%x\n",
__func__, i, chan_imr, chan_status);
@@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
*dev_id)
at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
- if (atchan->status & (AT_XDMAC_CIS_RBEIS |
AT_XDMAC_CIS_WBEIS))
+ if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS |
AT_XDMAC_CIS_WBEIS))
at_xdmac_write(atxdmac, AT_XDMAC_GD,
atchan->mask);
tasklet_schedule(&atchan->tasklet);
--
2.17.1
Re: [PATCH] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
On 20.01.2019 13:04, Vinod Koul wrote:
> Hi Codrin,
>
> On 17-01-19, 16:10, codrin.ciubota...@microchip.com wrote:
>> From: Codrin Ciubotariu
>>
>> atchan->status is used for two things:
>> - pass channel interrupts status from interrupt handler to tasklet;
>> - channel information like whether it is cyclic or paused;
>>
>> Since these operations have nothing in common, this patch adds a
>> different struct member to keep the interrupts status.
>>
>> Fixes a bug in which a channel is wrongfully reported as in use when
>> trying to obtain a new descriptior for a previously used cyclic channel.
>
> This looks reasonable but am unable to see how the bug is fixed. Perhaps
> would be great to split the bug part (which can go to fixes) and remove
> the reuse of variable as a subsequent patch..
Hi Vinod,
This patch is the fix, since it moves the operations on atchan->status,
in which the interrupt status register is saved, to a different member
(irq_status). The AT_XDMAC_CHAN_IS_CYCLIC and AT_XDMAC_CHAN_IS_PAUSED
bits have nothing in common with the interrupt status register.
The bug reproduces when a device_terminate_all() is called,
(AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End
of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of
atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when
a new descriptor for a cyclic transfer is created, the driver reports
the channel as in use:
if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
dev_err(chan2dev(chan), "channel currently used\n");
return NULL;
}
I can send v2 if you consider the commit message misleading.
Best regards,
Codrin
>
>>
>> Signed-off-by: Codrin Ciubotariu
>> ---
>> drivers/dma/at_xdmac.c | 19 ++-
>> 1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
>> index 4e557684f792..fe69dccfa0c0 100644
>> --- a/drivers/dma/at_xdmac.c
>> +++ b/drivers/dma/at_xdmac.c
>> @@ -203,6 +203,7 @@ struct at_xdmac_chan {
>> u32 save_cim;
>> u32 save_cnda;
>> u32 save_cndc;
>> +u32 irq_status;
>> unsigned long status;
>> struct tasklet_struct tasklet;
>> struct dma_slave_config sconfig;
>> @@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
>> struct at_xdmac_desc*desc;
>> u32 error_mask;
>>
>> -dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
>> - __func__, atchan->status);
>> +dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
>> +__func__, atchan->irq_status);
>>
>> error_mask = AT_XDMAC_CIS_RBEIS
>> | AT_XDMAC_CIS_WBEIS
>> @@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
>>
>> if (at_xdmac_chan_is_cyclic(atchan)) {
>> at_xdmac_handle_cyclic(atchan);
>> -} else if ((atchan->status & AT_XDMAC_CIS_LIS)
>> - || (atchan->status & error_mask)) {
>> +} else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
>> + || (atchan->irq_status & error_mask)) {
>> struct dma_async_tx_descriptor *txd;
>>
>> -if (atchan->status & AT_XDMAC_CIS_RBEIS)
>> +if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
>> dev_err(chan2dev(&atchan->chan), "read bus error!!!");
>> -if (atchan->status & AT_XDMAC_CIS_WBEIS)
>> +if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
>> dev_err(chan2dev(&atchan->chan), "write bus error!!!");
>> -if (atchan->status & AT_XDMAC_CIS_ROIS)
>> +if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
>> dev_err(chan2dev(&atchan->chan), "request overflow
>> error!!!");
>>
>> spin_lock(&atchan->lock);
>> @@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
>> *dev_id)
>> atchan = &atxdmac->chan[i];
>> chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
>> chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
>> -atchan->status = chan_status & chan_imr;
>> +atchan->irq_status = chan_status & chan_imr;
>> dev_vdbg(atxdmac->dma.dev,
>> "%s: chan%d: imr=0x%x, status=0x%x\n",
>> __func__, i, chan_imr, chan_status);
>> @@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
>> *dev_id)
>> at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
>> at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
>>
>> -if (atchan->status & (AT_XDMAC_CIS_RBEIS |
>> AT_XDMAC_CIS_WBEIS))
>> +
[PATCH] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
From: Codrin Ciubotariu
atchan->status is used for two things:
- pass channel interrupts status from interrupt handler to tasklet;
- channel information like whether it is cyclic or paused;
Since these operations have nothing in common, this patch adds a
different struct member to keep the interrupts status.
Fixes a bug in which a channel is wrongfully reported as in use when
trying to obtain a new descriptior for a previously used cyclic channel.
Signed-off-by: Codrin Ciubotariu
---
drivers/dma/at_xdmac.c | 19 ++-
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
index 4e557684f792..fe69dccfa0c0 100644
--- a/drivers/dma/at_xdmac.c
+++ b/drivers/dma/at_xdmac.c
@@ -203,6 +203,7 @@ struct at_xdmac_chan {
u32 save_cim;
u32 save_cnda;
u32 save_cndc;
+ u32 irq_status;
unsigned long status;
struct tasklet_struct tasklet;
struct dma_slave_config sconfig;
@@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
struct at_xdmac_desc*desc;
u32 error_mask;
- dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
-__func__, atchan->status);
+ dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
+ __func__, atchan->irq_status);
error_mask = AT_XDMAC_CIS_RBEIS
| AT_XDMAC_CIS_WBEIS
@@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
if (at_xdmac_chan_is_cyclic(atchan)) {
at_xdmac_handle_cyclic(atchan);
- } else if ((atchan->status & AT_XDMAC_CIS_LIS)
- || (atchan->status & error_mask)) {
+ } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
+ || (atchan->irq_status & error_mask)) {
struct dma_async_tx_descriptor *txd;
- if (atchan->status & AT_XDMAC_CIS_RBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
dev_err(chan2dev(&atchan->chan), "read bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_WBEIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
dev_err(chan2dev(&atchan->chan), "write bus error!!!");
- if (atchan->status & AT_XDMAC_CIS_ROIS)
+ if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
dev_err(chan2dev(&atchan->chan), "request overflow
error!!!");
spin_lock(&atchan->lock);
@@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
*dev_id)
atchan = &atxdmac->chan[i];
chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
- atchan->status = chan_status & chan_imr;
+ atchan->irq_status = chan_status & chan_imr;
dev_vdbg(atxdmac->dma.dev,
"%s: chan%d: imr=0x%x, status=0x%x\n",
__func__, i, chan_imr, chan_status);
@@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void
*dev_id)
at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
- if (atchan->status & (AT_XDMAC_CIS_RBEIS |
AT_XDMAC_CIS_WBEIS))
+ if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS |
AT_XDMAC_CIS_WBEIS))
at_xdmac_write(atxdmac, AT_XDMAC_GD,
atchan->mask);
tasklet_schedule(&atchan->tasklet);
--
2.17.1
